Fix HttpListener restart - #41867
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
stephentoub
left a comment
stephentoub
left a comment
There was a problem hiding this comment.
Other than the naming/tracing comments, LGTM.
| if (_requestQueueBoundHandle != null) | ||
| { | ||
| _requestQueueBoundHandle.Dispose(); | ||
| _requestQueueBoundHandle = null; |
There was a problem hiding this comment.
It looks like setting this to null could, with certain interleavings, cause:
to return null. Might this cause a null reference exception elsewhere? Callers of that property expect it to always return non-null.
There was a problem hiding this comment.
I think you're right; if someone calls Stop() at just the right moment, RequestQueueBoundHandle might return null.
I can fix this -- if I include the return _requestQueueBoundHandle in the lock, then we won't NRE due to this. It creates a new situation where we might allocate a new one that doesn't get disposed, but GC will take care of that and Dispose is a no-op right now so that is fine regardless.
This is related to one of those race conditions I alluded to. With this fixed, those other things will still get an error because they all try to, immediately after getting the ThreadPoolBoundHandle, use the _requestQueueHandle that is being disposed right after the _requestQueueBoundHandle. (╯°□°）╯︵ ┻━┻
There was a problem hiding this comment.
(╯°□°）╯︵ ┻━┻
What is that? :)
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks. Obviously I need to take an updated emoticon course to learn these things. :)
There was a problem hiding this comment.
This is related to one of those race conditions I alluded to. With this fixed, those other things will still get an error because they all try to, immediately after getting the ThreadPoolBoundHandle, use the _requestQueueHandle that is being disposed right after the _requestQueueBoundHandle.
So does that mean this PR won't make things worse (and obviously will make one case better), or are there cases where this will turn things that were previously nops into failures?
There was a problem hiding this comment.
So does that mean this PR won't make things worse (and obviously will make one case better), or are there cases where this will turn things that were previously nops into failures?
The basic race when stopping in HttpListener is:
- An outstanding multi-step op completes its first leg successfully.
- You call Stop() and close everything.
- The outstanding op starts its second leg and hopefully the handle was set to
NULLorINVALID_HANDLEso the http.sys function can error out.
This is what exists today and this PR won't change that.
In .NET 4.8 there's an additional race, which doesn't effect .NET Core because we don't support restarting:
- An outstanding multi-step op completes its first leg successfully and right before its second leg, the thread ends its quantum.
- You call Stop() and close everything.
- You call Start() again.
- Thread resumes and the outstanding op starts its second leg and it passes now invalid state to http.sys functions.
This PR will re-enable this 2nd race.
| _requestQueueBoundHandle = ThreadPoolBoundHandle.BindHandle(_requestQueueHandle); | ||
| if (NetEventSource.IsEnabled) NetEventSource.Info($"ThreadPoolBoundHandle.BindHandle({_requestQueueHandle}) -> {_requestQueueBoundHandle}"); | ||
| } | ||
| _requestQueueBoundHandle = handle = ThreadPoolBoundHandle.BindHandle(_requestQueueHandle); |
There was a problem hiding this comment.
Can the updated DisconnectAsyncResult ctor that now calls into this getter ever run after CloseRequestQueueHandle completes? I'm not familiar with the HttpListener code, but it looks like RegisterForDisconnectNotification isn't synchronized.
Could this risk BindHandle throwing a new ArgumentException since _requestQueueHandle would be closed at that point?
|
Thank you for your contribution. As announced in dotnet/coreclr#27549 this repository will be moving to dotnet/runtime on November 13. If you would like to continue working on this PR after this date, the easiest way to move the change to dotnet/runtime is:
|
|
We've decided to invest time into more thorough fix of |
7 participants
Resolves #39552 throwing
ObjectDisposedException.The first try in #40466 failed due to the
ThreadPoolBoundHandlebeing set to null before theDisconnectAsyncResultcallback was hit. This resolves that by keeping a private copy of theThreadPoolBoundHandlearound inside theDisconnectAsyncResult.This is technically a use-after-Dispose, relying on our current implementation of
ThreadPoolBoundHandlewhich happens to allow this, so the fix may not work long-term. A more correct fix needs more thought and is blocked on https://github.com/dotnet/coreclr/issues/26783 due to how http.sys implements cancellation, which asks us to close the handle to cancel but then requires us to use the handle to free ourOVERLAPPEDstructure.Socketimplementation might be doing something similar so that's a good place to reference.