-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Race condition causes header frame to be fired on HTTP2StreamChannel AFTER first data frame(s) #410
Comments
From a quick look this looks like a reentrancy problem where as you pointed out we can re-order reads. This is a bit of an interesting one since anything can potentially happen between these two method calls
I am wondering if we could just reorder those two calls so that the read gets buffered before we configure the stream channel; however, I am deferring this to @Lukasa who is more familiar with the insides of our H2 stack. |
Note that
So I think the reason this hasn't happened to anyone else is that it's very uncommon for Franz's suggestion of a fix seems appropriate: we can safely buffer up that headers frame and then hit the configure flow. @qusc would you like to write a test for this? You'll need to use a custom |
…on causing incorrect order of inbound `HTTP2Frame`s Motivation: Make sure the order of `HTTP2Frame`s that are fired through the pipeline of a `HTTP2StreamChannel` by `HTTP2CommonInboundStreamMultiplexer` is correct when a `read()` call on the parent channel synchronously causes further frames to be processed. Modifications: Reorder calls in `HTTP2CommonInboundStreamMultiplexer.receivedFrame(frame:context:multiplexer)` so that initial header frame is buffered and processed first. Result: Resolves apple#410 and makes newly added test case `HTTP2StreamMultiplexerTests.testMultiplexerFiresInitialFramesInCorrectOrder()` pass.
Thanks for your support! Let me know if the fix and test case I added to my pull request are implemented appropriately @Lukasa |
…on causing incorrect order of inbound `HTTP2Frame`s Motivation: Make sure the order of `HTTP2Frame`s that are fired through the pipeline of a `HTTP2StreamChannel` by `HTTP2CommonInboundStreamMultiplexer` is correct when a `read()` call on the parent channel synchronously causes further frames to be processed. Modifications: Reorder calls in `HTTP2CommonInboundStreamMultiplexer.receivedFrame(frame:context:multiplexer)` so that initial header frame is buffered and processed first. Result: Resolves apple#410 and makes newly added test case `HTTP2StreamMultiplexerTests.testMultiplexerFiresInitialFramesInCorrectOrder()` pass.
…on causing incorrect order of inbound `HTTP2Frame`s Motivation: Make sure the order of `HTTP2Frame`s that are fired through the pipeline of a `HTTP2StreamChannel` by `HTTP2CommonInboundStreamMultiplexer` is correct when a `read()` call on the parent channel synchronously causes further frames to be processed. Modifications: Reorder calls in `HTTP2CommonInboundStreamMultiplexer.receivedFrame(frame:context:multiplexer)` so that initial header frame is buffered and processed first. Result: Resolves apple#410 and makes newly added test case `HTTP2StreamMultiplexerTests.testMultiplexerFiresInitialFramesInCorrectOrder()` pass.
…on causing incorrect order of inbound `HTTP2Frame`s Motivation: Make sure the order of `HTTP2Frame`s that are fired through the pipeline of a `HTTP2StreamChannel` by `HTTP2CommonInboundStreamMultiplexer` is correct when a `read()` call on the parent channel synchronously causes further frames to be processed. Modifications: Reorder calls in `HTTP2CommonInboundStreamMultiplexer.receivedFrame(frame:context:multiplexer)` so that initial header frame is buffered and processed first. Result: Resolves apple#410 and makes newly added test case `HTTP2StreamMultiplexerTests.testMultiplexerFiresInitialFramesInCorrectOrder()` pass.
…on causing incorrect order of inbound `HTTP2Frame`s (#413) Motivation: Make sure the order of `HTTP2Frame`s that are fired through the pipeline of a `HTTP2StreamChannel` by `HTTP2CommonInboundStreamMultiplexer` is correct when a `read()` call on the parent channel synchronously causes further frames to be processed. Modifications: Reorder calls in `HTTP2CommonInboundStreamMultiplexer.receivedFrame(frame:context:multiplexer)` so that initial header frame is buffered and processed first. Result: Resolves #410 and makes newly added test case `HTTP2StreamMultiplexerTests.testMultiplexerFiresInitialFramesInCorrectOrder()` pass.
Hi, I have a pretty weird bug going on, not sure if I'm doing something wrong but I thought I'd share this. I'm on tag 1.17.0. It's fairly easy for me to reproduce but I have quite a lot of custom code involved that I cannot easily share. I'll do my best though to describe what I think is happening.
So whenever we get a new decoded header frame in
HTTP2CommonInboundStreamMultiplexer::receivedFrame(_:, context:, multiplexer:)
swift-nio-http2/Sources/NIOHTTP2/HTTP2CommonInboundStreamMultiplexer.swift
Line 92 in 044339d
we create a new stream channel and trigger its initializer:
swift-nio-http2/Sources/NIOHTTP2/HTTP2CommonInboundStreamMultiplexer.swift
Line 112 in 044339d
Then, we fire the header we got through the pipeline of this new channel as a first frame
swift-nio-http2/Sources/NIOHTTP2/HTTP2CommonInboundStreamMultiplexer.swift
Line 113 in 044339d
The problem seems to be, that within the initialization of the new stream channel, eventually
HTTP2StreamChannel::performActivation()
will be calledswift-nio-http2/Sources/NIOHTTP2/HTTP2StreamChannel.swift
Line 305 in 044339d
then
swift-nio-http2/Sources/NIOHTTP2/HTTP2StreamChannel.swift
Line 707 in 044339d
which will trigger another read on the pipeline:
swift-nio-http2/Sources/NIOHTTP2/HTTP2StreamChannel.swift
Line 712 in 044339d
That in turn might cause another frame to be processed in
swift-nio-http2/Sources/NIOHTTP2/HTTP2CommonInboundStreamMultiplexer.swift
Line 70 in 044339d
recursively and fire that second frame through the pipeline of the now existent (in
HTTP2CommonInboundStreamMultiplexer::streams
) stream channel! The first (header) frame payload is still to be delivered sinceHTTP2CommonInboundStreamMultiplexer::receivedFrame(...)
is still on the call stack, waiting for the initializer of the new stream channel to return here:swift-nio-http2/Sources/NIOHTTP2/HTTP2CommonInboundStreamMultiplexer.swift
Line 112 in 044339d
No idea why this bug wouldn't have appeared for anyone else earlier. Seems pretty fundamental. What could I be doing wrong here? I'm using a custom pipeline underneath the HTTP2 handler which implements a proxy connection / virtual socket.
Wrapping
self.tryToRead()
andself.deliverPendingWrites()
ineventLoop.execute()
like soin
swift-nio-http2/Sources/NIOHTTP2/HTTP2StreamChannel.swift
Line 305 in 044339d
so that new frames are only read after the header has finished processing seems to solve the issue for me.
The text was updated successfully, but these errors were encountered: