Don't reuse the same exception through multiple paths #11827

benaadams · 2019-07-03T01:09:48Z

Am guessing this is the cause of the long chain in #11804 (though doesn't fix the underlying issue)

crit: Microsoft.AspNetCore.Server.Kestrel[0]
      Http2OutpuProducer.ProcessDataWrites() observed an unexpected exception.
System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values.
   at System.IO.Pipelines.BufferSegment.set_End(Int32 value)
   at System.IO.Pipelines.Pipe.CommitUnsynchronized()
   at System.IO.Pipelines.Pipe.PrepareFlush(CompletionData& completionData, ValueTask`1& result, CancellationToken cancellationToken)
   at System.IO.Pipelines.Pipe.FlushAsync(CancellationToken cancellationToken)
   at System.IO.Pipelines.Pipe.DefaultPipeWriter.FlushAsync(CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.TimeFlushAsync(MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2OutputProducer.ProcessDataWrites()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2OutputProducer.ProcessDataWrites()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2OutputProducer.ProcessDataWrites()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2OutputProducer.ProcessDataWrites()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2OutputProducer.ProcessDataWrites()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2OutputProducer.ProcessDataWrites()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2OutputProducer.ProcessDataWrites()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2OutputProducer.ProcessDataWrites()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2OutputProducer.ProcessDataWrites()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2OutputProducer.ProcessDataWrites()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2OutputProducer.ProcessDataWrites()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2OutputProducer.ProcessDataWrites()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2OutputProducer.ProcessDataWrites()
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure.TimingPipeFlusher.AwaitLastFlushAndTimeFlushAsync(ValueTask`1 lastFlushTask, MinDataRate minRate, Int64 count, IHttpOutputAborter outputAborter, CancellationToken cancellationToken)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.Http2OutputProducer.ProcessDataWrites()

/cc @halter73 @davidfowl @stephentoub

halter73 · 2019-07-03T01:42:19Z

It looks like there are 10 Http2Stream tests failing because they expect an exception type other than a ConnectionAbortedException. I think we should just update the tests to reflect the new behavior.

benaadams · 2019-07-03T02:50:11Z

Because ConnectionAbortedException inherits from OperationCanceledException it becomes a TaskCanceledException (with an inner exception of ConnectionAbortedException)

That still work?

halter73 · 2019-07-03T03:02:21Z

We should ensure that server-initiated aborts result in Task/OperationCanceledExceptions and client-initiated aborts/resets result in IOExceptions. The common abort path might have led to us getting this wrong in a few places.

benaadams · 2019-07-03T14:01:12Z

Done. There's a problem with pipelines also

benaadams · 2019-07-03T14:20:34Z

Fix for Pipelines exceptions dotnet/corefx#39158

benaadams · 2019-07-03T16:24:28Z

Rebased to kick off the helix-test again (IIS issues)

benaadams · 2019-07-03T21:04:27Z

Betterized it

halter73 · 2019-07-03T21:16:00Z

src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs

+
+            // Unblock the request body.
+            PoisonRequestBodyStream(new IOException(abortReason.Message, abortReason));
+            RequestBodyPipe.Writer.Complete(new IOException(abortReason.Message, abortReason));


I don't think we really need to new up two exceptions here, since there should only be one reader accessing a single HTTP/2 request body at a time.

@Tratcher made a good point that it might be better to have the caller of Abort or ResetAndAbort clone the exceptions if needed. After all, it's the caller that knows if the exception is being used to complete multiple streams. As it is, this seems a little overly defensive, though certainly better than what's currently in master.

Except its used by Request PoisonRequestBodyStream and Response Writer.Complete which can happen in parallel?

Sure, but it's only a bug if it get's thrown/handled in parallel, right? There's only one reader for each stream who will either observe the exception via the poisoned request body stream OR the poisoned request body pipe.

The reader could only observe both exceptions it swallowed the exception from the pipe first and then tried reading again anyway thereby observing the the exception from the poisoned stream. This wouldn't cause parallel access to the exception though.

The only obvious place I can see where it will keep adding to the exception stack trace is in HttpRequestPipeReader #11875 but I would have thought that would have shown up in the stack trace?

Http2Connection already creates a new exception for each Http2Stream https://github.com/aspnet/AspNetCore/blob/3f73a0fdcff217d061b994f3bd6528f1cd8caf2e/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs#L279-L282

However the stack trace would suggest either its happening in parallel or its chaining Write+Flush on Write+Flush on Write+Flush without ever waiting for any completion; so an earlier Flush would be active concurrently with a later Write?

I really want to find the root of the problem. The exception set here should only be observed once or maybe repeatedly thrown to one caller if the caller happens swallow the exception. I really doubt that we're adding to the exception stack trace because we're throwing from both the poisoned Stream and PipeReader.

Question is, do you think that could be an accurate the stacktrace?

Question is, do you think that could be an accurate the stacktrace?

I hope not, but I haven't ruled it out completely yet.

Once #12081 is merged, TimingPipeFlusher will no longer be maintaining a flush Task queue. Instead ConcurrentQueue will be doing this job.

I think ConcurrentQueue stack traces will be easier to reason about because there is only ever one TCS at a time configured with TaskCreationOptions.RunContinuationsAsynchronously instead of an arbitrarily long linked list of Tasks being awaited.

src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs

src/Servers/Kestrel/Core/src/Internal/Infrastructure/BodyControl.cs

benaadams · 2019-07-18T13:10:50Z

Going to close this one then

benaadams requested review from analogrelay, halter73, jkotalik and Tratcher as code owners July 3, 2019 01:09

stephentoub approved these changes Jul 3, 2019

View reviewed changes

analogrelay added the area-servers label Jul 3, 2019

analogrelay added this to the 3.0.0-preview8 milestone Jul 3, 2019

halter73 approved these changes Jul 3, 2019

View reviewed changes

benaadams force-pushed the Exceptions branch from 03bea2f to 5223dde Compare July 3, 2019 02:47

benaadams added 3 commits July 3, 2019 17:23

Don't reuse the same exception through multiple paths

c00ce1b

React tests

a4bd4c4

Differentiate client and server aborts

df804e0

benaadams force-pushed the Exceptions branch from 33612d9 to df804e0 Compare July 3, 2019 16:23

halter73 self-requested a review July 3, 2019 18:30

benaadams mentioned this pull request Jul 3, 2019

Stop using EDI.Throw for exception that may be thrown multiple times in Pipelines dotnet/corefx#39158

Closed

halter73 reviewed Jul 3, 2019

View reviewed changes

halter73 self-requested a review July 3, 2019 21:22

benaadams force-pushed the Exceptions branch from bfb7356 to a1d7283 Compare July 3, 2019 21:33

Capture ExceptionDispatchInfo at Abort not throw

ff2673a

benaadams force-pushed the Exceptions branch from a1d7283 to ff2673a Compare July 3, 2019 21:34

Code check

cc5db71

benaadams mentioned this pull request Jul 5, 2019

Capture Edi at Abort not throw #11875

Merged

halter73 reviewed Jul 12, 2019

View reviewed changes

src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs Show resolved Hide resolved

halter73 reviewed Jul 17, 2019

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Infrastructure/BodyControl.cs Show resolved Hide resolved

benaadams closed this Jul 18, 2019

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

Don't reuse the same exception through multiple paths #11827

Don't reuse the same exception through multiple paths #11827

Uh oh!

Conversation

benaadams commented Jul 3, 2019

Uh oh!

halter73 commented Jul 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Jul 3, 2019

Uh oh!

halter73 commented Jul 3, 2019

Uh oh!

benaadams commented Jul 3, 2019

Uh oh!

benaadams commented Jul 3, 2019

Uh oh!

benaadams commented Jul 3, 2019

Uh oh!

benaadams commented Jul 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

benaadams commented Jul 18, 2019

Uh oh!

Uh oh!

halter73 commented Jul 3, 2019 •

edited

Loading