Don't post to closed socket (CI Fix)

[benaadams]

Resolves #336

[benaadams]

Also resolves #259

[halter73]

I agree that #336 and #259 are likely the same issue.

I had considered trying a fix like this, but I don't think checking SafeHandle.IsClosed or SafeHandle.IsInvalid is sufficient to avoid the AVs. SafeHandles should throw an ODE if used after being closed like the following:

System.ObjectDisposedException: Safe handle has been closed
   at System.Runtime.InteropServices.SafeHandle.DangerousAddRef(Boolean& success)
   at System.StubHelpers.StubHelpers.SafeHandleAddRef(SafeHandle pHandle, Boolean& success)
   at Microsoft.AspNet.Server.Kestrel.Networking.Libuv.NativeMethods.uv_async_send(UvAsyncHandle handle)
   at Microsoft.AspNet.Server.Kestrel.Networking.Libuv.async_send(UvAsyncHandle handle) in C:\Users\shalter\dev\Universe\KestrelHttpServer\src\Microsoft.AspNet.Server.Kestrel\Networking\Libuv.cs:line 202
   at Microsoft.AspNet.Server.Kestrel.Networking.UvAsyncHandle.Send() in C:\Users\shalter\dev\Universe\KestrelHttpServer\src\Microsoft.AspNet.Server.Kestrel\Networking\UvAsyncHandle.cs:line 37

This isn't the worst thing as long as long as we wrap the ODE in an IOException (#299).

I actually modified the AsyncCanBeSent test to try to force the AV and ran it for a couple hours today, but I had no luck:

[Fact]
public void AsyncCanBeSent()
{
    UvAsyncHandle triggerRef = null;
    var tasks = new Task[2];

    tasks[0] = Task.Run(() =>
    {
        while (true)
        {
            var loop = new UvLoopHandle(_logger);
            loop.Init(_uv);
            var trigger = new UvAsyncHandle(_logger);
            trigger.Init(loop, () =>
            {
                trigger.Dispose();
            });

            triggerRef = trigger;

            trigger.Send();
            loop.Run();
            loop.Dispose();
        }
    });

    tasks[1] = Task.Run(() =>
    {
        while (true)
        {
            if (triggerRef != null)
            {
                try
                {
                    triggerRef.Send();
                }
                catch (ObjectDisposedException ex)
                {
                }
            }
        }
    });

    Task.WhenAny(tasks).Result.Wait();
}

I think there's more to this than meets the eye.

[halter73]

If this was intended to prevent AVs, it could only have any affect on debug builds. I don't think even Trace.Assert would save us from the AVs we've been seeing.

This might be nice to have, but I doubt this fixes any bugs.

[benaadams]

Helped me track down the bug; only needed for debug.

[benaadams]

If you put just this change in and run ./build in Windows; debug when the Assert pops up you can clearly see what's going on.

[benaadams]

Its when loop has been shutdown so trying to queue socketDisconnect fails; because the client shut it down as part of socketShutdownSend and the loop picked it up, then decided it had no outstanding work so was safe to shutdown.

[benaadams]

The tests will cause this to occur more than I imagine it would happen in the wild as I think it only occurs on shutdown.

[benaadams]

Basically this fix is don't post to terminated loop; but the socket is also closed and is a more local check.

[benaadams]

This is too annoying, rebasing other back PRs onto it.

[Tragetaschen]

@davidfowl That what "callers know" is normally the Connection, but the current design of Kestrel fires and forgets any client request and knowledge about active connections. I'd make an educated guess that this decision is at the bottom of all this. That's for example the reason, the rude shutdown method of the loop exists in the first place, because there is nothing to properly shut down active connections for example on the Frame level. Instead it's done at the socket level.

[benaadams]

@Tragetaschen not really, the problem is caused by Kestrel being well behaved; and the client being local and 0 ms away.

So Kestrel waits for the "socket shutdown" message to be "on the wire" before tearing down the socket, otherwise the client will never receive it and will remain in a connected state; however the client receives that message instantly so initiates its own shutdown, then libuv cleans up all before the confirm of the message being on the wire comes back; at which point Kestrel then tries to dispose the socket and fails because everything is already gone; which includes the libuv loop as its being shut down.

[benaadams]

@Tragetaschen also looks like your other issue is addressed by this: IHttpRequestLifetimeFeature #325

[halter73]

If you take my AsyncCanBeSent test case and replace trigger.Dispose() with trigger.DangerousClose() like we call in KestrelThread.ThreadStart while tearing down, the AV repros immediately.

I think the call to DangerousClose was meant to prevent ODE's from being thrown from KestrelThread.Stop, but the way it is currently written there will always be a race condition in that code.

I think for now it's best to catch the potential ODEs in KestrelThread.Stop and simply call UvAsyncHandle.Dispose inside of KestrelThread.ThreadStart.

We can then get rid of UvAsyncHandle.DangerousClose. It's way too dangerous for me 😨

@lodejard @davidfowl @benaadams What do you all think?

Here's my first attempt at a PR: #347

[benaadams]

Better fix by @halter73 in #347, closing this