Unskip and fix race in ConnectionClosedEvenIfAppSwallowsException #2632
Conversation
| systemClock.UtcNow += gracePeriod + TimeSpan.FromSeconds(1); | ||
| Assert.True(exceptionSwallowedEvent.Wait(TestConstants.DefaultTimeout), "ExceptionSwallowedEvent timed out."); | ||
| await exceptionSwallowedTcs.Task.DefaultTimeout(); |
There was a problem hiding this comment.
What are we asserting now? If there's nothing that we should/can assert I generally like to leave a comment noting the exception we're confirming we don't it, or something along those lines. That helps make it clear that we didn't just forget the Assert (which I've seen in our code-base), it was an active choice.
There was a problem hiding this comment.
DefaultTimeout() is an assertion. This is common in Kestrel functional tests. Another common assertion is awaiting a TCS where SetException is set in the app func. Adding comments everywhere pointing out this is an assertion would be super noisy.
There was a problem hiding this comment.
Same goes for Receive[ForcedEnd]. That's also an assertion that we use everywhere. Anyone who works at all with Kestrel functional tests will be familiar with this convention.
|
Just noticed that the CI failed. Looks like |
|
@ryanbrandenburg That's a a problem we see a lot in our macOS libuv functional tests. It's not consistent at all which test fails, but it always surfaces as an empty response. The test ran twice with the libuv transport. You can see that in the log for the successful test run, the request is handled, a response is written, and only after that does the server close the connection as indicated by the "disconnecting" and "sending FIN" logs. In the log for the failed test run the client sends a FIN immediately after opening connection prior to ever sending a request, so the server appropriately closes the connection immediately. This is indicated by the "received FIN" log by the server. This can only be logged if a -4095 status is observed by the libuv transport's read callback. Not only is the client not supposed to send a FIN prior to sending the request (which is obvious) it's also supposed to wait to send a FIN until after it receives a complete response, so there's no way this should be happening. I would say it has to be a client bug except the client in this case is Kestrel's |
|
@ryanbrandenburg I created a single issue to track this class of failures at #2635. If you know of any other issues you filed that relates to this behavior, it would be helpful if you could mark those as dupes of #2635 and close them. |
|
Thanks for the quick approval @ryanbrandenburg |
#2464
This is the same fix @mikeharder made for RequestTimesOutWhenRequestBodyNotReceivedAtSpecifiedMinimumRate with #2589.