Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Don't post to closed socket (CI Fix) #337

Closed
wants to merge 2 commits into from
Closed

Don't post to closed socket (CI Fix) #337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cad9980
Don't post to closed socket
benaadams Nov 6, 2015
1dea9f9
Error all outstanding callbacks on socket error
benaadams Nov 6, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion src/Microsoft.AspNet.Server.Kestrel/Http/SocketOutput.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,15 @@ public void End(ProduceEndType endType)

private void ScheduleWrite()
{
_thread.Post(_this => _this.WriteAllPending(), this);
// Don't post write to closed socket
if (!_socket.IsClosed)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to be a perf optimization? _thread.Post shouldn't fail because the socket is closed.

We only actually use _socket in the WriteContext methods where _socket.IsClosed is always checked first thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is the loop is the gone hence all the calls erroring are actually uv_async_send so its post that fails. However the socket has been successfully closed on the server; so a check to see if the socket is closed fixes it; rather than checking if the loop is valid.

It occurs when socketShutdownSend has happened; the client closes the socket, the server picks this up and then the close is kicked off; so when there is a slight delay between socketShutdownSend and socketDisconnect while the loop has terminated because its been asked to and all the sockets are closed.

Its potentially only in a full shutdown scenarios (e.g. the tests or controlled app recycle)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better to expose something from KestrelThread so that callers know?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its only in a shutdown race situation; happens in both windows and mono. When it happens windows seems ok with it, but mono very unhappy.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for example OnWriteCompleted calls it after it acquires the lock as does Write after doing a bunch of stuff

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will mitigate this in a later PR; but this will stop the AV for now.

{
_thread.Post(_this => _this.WriteAllPending(), this);
}
else
{
CompleteAllCallbacksWithError(_lastWriteError ?? new InvalidOperationException("Socket is closed"));
}
}

// This is called on the libuv event loop
Expand Down Expand Up @@ -173,6 +181,20 @@ private void WriteAllPending()
throw;
}
}

private void CompleteAllCallbacksWithError(Exception error)
{
lock (_lockObj)
{
while (_callbacksPending.Count > 0)
{
var callbackContext = _callbacksPending.Dequeue();

// callback(error, state, calledInline)
callbackContext.Callback(error, callbackContext.State, false);
}
}
}

// This is called on the libuv event loop
private void OnWriteCompleted(Queue<ArraySegment<byte>> writtenBuffers, int status, Exception error)
Expand Down
4 changes: 4 additions & 0 deletions src/Microsoft.AspNet.Server.Kestrel/Networking/Libuv.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Diagnostics;
using System.Runtime.InteropServices;

namespace Microsoft.AspNet.Server.Kestrel.Networking
Expand Down Expand Up @@ -199,6 +200,9 @@ public void async_init(UvLoopHandle loop, UvAsyncHandle handle, uv_async_cb cb)
protected Func<UvAsyncHandle, int> _uv_async_send;
public void async_send(UvAsyncHandle handle)
{
// Can't Assert with .Validate as that checks threadId
// and this function is to post to correct thread.
Debug.Assert(!handle.IsInvalid, "Handle is invalid");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Check(_uv_async_send(handle));
}

Expand Down