Skip to content

Crash fix: HTTP2Connections emit events after the pool has closed them. #481

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 19, 2021
Merged

Crash fix: HTTP2Connections emit events after the pool has closed them. #481

merged 1 commit into from
Nov 19, 2021

Conversation

fabianfett
Copy link
Member

On the swift-server slack a crash was reported:

2021-11-19T04:19:37.87233336Z 0x55a5a551c262, Backtrace.(printBacktrace in _B82A8C0ED7C904841114FDF244F9E58E)(signal: Swift.Int32) -> () at /build/.build/checkouts/swift-backtrace/Sources/Backtrace/Backtrace.swift:66
2021-11-19T04:19:37.872355121Z 0x7f702ab0d3bf
2021-11-19T04:19:37.872771454Z 0x55a5a53cdda6, Swift runtime failure: precondition failure at .build/checkouts/async-http-client/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift:0
2021-11-19T04:19:37.872787184Z 0x55a5a53cdda6, generic specialization <Swift.Array<AsyncHTTPClient.HTTPConnectionPool.(HTTP2ConnectionState in _06AB5E1F2E49650D236C760F038E3F99)>> of (extension in Swift):Swift.Collection.firstIndex(where: (A.Element) throws -> Swift.Bool) throws -> Swift.Optional<A.Index> at .build/checkouts/async-http-client/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift:528
2021-11-19T04:19:37.872823265Z 0x55a5a53cdda6, AsyncHTTPClient.HTTPConnectionPool.HTTP2Connections.goAwayReceived(Swift.Int) -> AsyncHTTPClient.HTTPConnectionPool.HTTP2Connections.GoAwayContext at .build/checkouts/async-http-client/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift:527
2021-11-19T04:19:37.872830366Z 0x55a5a53cdda6, AsyncHTTPClient.HTTPConnectionPool.HTTP2StateMachine.http2ConnectionGoAwayReceived(Swift.Int) -> AsyncHTTPClient.HTTPConnectionPool.StateMachine.Action at .build/checkouts/async-http-client/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2StateMachine.swift:301
2021-11-19T04:19:37.872992441Z 0x55a5a53cdda6, closure #2 (inout AsyncHTTPClient.HTTPConnectionPool.HTTP2StateMachine) -> AsyncHTTPClient.HTTPConnectionPool.StateMachine.Action in AsyncHTTPClient.HTTPConnectionPool.StateMachine.http2ConnectionGoAwayReceived(Swift.Int) -> AsyncHTTPClient.HTTPConnectionPool.StateMachine.Action at /build/.build/checkouts/async-http-client/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift:178
2021-11-19T04:19:37.873006651Z 0x55a5a53cdda6, reabstraction thunk helper from @callee_guaranteed (@inout AsyncHTTPClient.HTTPConnectionPool.HTTP2StateMachine) -> (@owned AsyncHTTPClient.HTTPConnectionPool.StateMachine.Action) to @escaping @callee_guaranteed (@inout AsyncHTTPClient.HTTPConnectionPool.HTTP2StateMachine) -> (@out AsyncHTTPClient.HTTPConnectionPool.StateMachine.Action) at /build/<compiler-generated>:0
2021-11-19T04:19:37.873013791Z 0x55a5a53cdda6, generic specialization <AsyncHTTPClient.HTTPConnectionPool.StateMachine.Action> of AsyncHTTPClient.HTTPConnectionPool.StateMachine.HTTPVersionState.modify<A>(http1: (inout AsyncHTTPClient.HTTPConnectionPool.HTTP1StateMachine) -> A, http2: (inout AsyncHTTPClient.HTTPConnectionPool.HTTP2StateMachine) -> A) -> A at /build/.build/checkouts/async-http-client/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift:83
2021-11-19T04:19:37.873021172Z 0x55a5a53cdda6, AsyncHTTPClient.HTTPConnectionPool.StateMachine.http2ConnectionGoAwayReceived(Swift.Int) -> AsyncHTTPClient.HTTPConnectionPool.StateMachine.Action at /build/.build/checkouts/async-http-client/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift:175
2021-11-19T04:19:37.873026881Z 0x55a5a53aa6f9
2021-11-19T04:19:37.87460722Z 0x55a5a539b5b6, function signature specialization <Arg[0] = [Closure Propagated : closure #1 (inout AsyncHTTPClient.HTTPConnectionPool.StateMachine) -> AsyncHTTPClient.HTTPConnectionPool.StateMachine.Action in AsyncHTTPClient.HTTPConnectionPool.http2ConnectionGoAwayReceived(AsyncHTTPClient.HTTP2Connection) -> (), Argument Types : [AsyncHTTPClient.HTTP2Connection]> of AsyncHTTPClient.HTTPConnectionPool.(modifyStateAndRunActions in _77984E52C350E955F8640F5ED48B34F0)((inout AsyncHTTPClient.HTTPConnectionPool.StateMachine) -> AsyncHTTPClient.HTTPConnectionPool.StateMachine.Action) -> () at /build/<compiler-generated>:0
2021-11-19T04:19:37.87463004Z 0x55a5a539b5b6, AsyncHTTPClient.HTTPConnectionPool.http2ConnectionGoAwayReceived(AsyncHTTPClient.HTTP2Connection) -> () at /build/.build/checkouts/async-http-client/Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool.swift:511
2021-11-19T04:19:37.874983331Z 0x55a5a538242f, protocol witness for AsyncHTTPClient.HTTP2ConnectionDelegate.http2ConnectionGoAwayReceived(AsyncHTTPClient.HTTP2Connection) -> () in conformance AsyncHTTPClient.HTTPConnectionPool : AsyncHTTPClient.HTTP2ConnectionDelegate in AsyncHTTPClient at /build/<compiler-generated>:0
2021-11-19T04:19:37.874999892Z 0x55a5a538242f, AsyncHTTPClient.HTTP2Connection.http2GoAwayReceived() -> () at /build/.build/checkouts/async-http-client/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2Connection.swift:294
2021-11-19T04:19:37.875006062Z 0x55a5a53824da, protocol witness for AsyncHTTPClient.HTTP2IdleHandlerDelegate.http2GoAwayReceived() -> () in conformance AsyncHTTPClient.HTTP2Connection : AsyncHTTPClient.HTTP2IdleHandlerDelegate in AsyncHTTPClient at /build/<compiler-generated>:0
2021-11-19T04:19:37.875216778Z 0x55a5a5385f94, AsyncHTTPClient.HTTP2IdleHandler.(run in _3ECE02A98D9CDE93B24E9B53920C25AB)(_: AsyncHTTPClient.HTTP2IdleHandler<A>.StateMachine.Action, context: NIOCore.ChannelHandlerContext) -> () at /build/.build/checkouts/async-http-client/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift:124
2021-11-19T04:19:37.875241448Z 0x55a5a5385f94, AsyncHTTPClient.HTTP2IdleHandler.channelRead(context: NIOCore.ChannelHandlerContext, data: NIOCore.NIOAny) -> () at /build/.build/checkouts/async-http-client/Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2IdleHandler.swift:67
2021-11-19T04:19:37.880177408Z 0x55a5a5b76c92, NIOCore.ChannelHandlerContext.(invokeChannelRead in _F5AC316541457BD146E3694279514AA3)(NIOCore.NIOAny) -> () at /build/.build/checkouts/swift-nio/Sources/NIOCore/ChannelPipeline.swift:1688
2021-11-19T04:19:37.880201319Z 0x55a5a5b7478f, NIOCore.ChannelHandlerContext.fireChannelRead(NIOCore.NIOAny) -> () at /build/.build/checkouts/swift-nio/Sources/NIOCore/ChannelPipeline.swift:1501
2021-11-19T04:19:37.881153888Z 0x55a5a5c5cc7f, NIOHTTP2.NIOHTTP2Handler.(processFrame in _57CB3A28E3B2E42C5D0799905DE90E42)(_: NIOHTTP2.HTTP2Frame, flowControlledLength: Swift.Int, context: NIOCore.ChannelHandlerContext) -> NIOHTTP2.NIOHTTP2Handler.FrameProcessResult at /build/.build/checkouts/swift-nio-http2/Sources/NIOHTTP2/HTTP2ChannelHandler.swift:299
2021-11-19T04:19:37.881170398Z 0x55a5a5c59fb8, NIOHTTP2.NIOHTTP2Handler.(frameDecodeLoop in _57CB3A28E3B2E42C5D0799905DE90E42)(context: NIOCore.ChannelHandlerContext) -> () at /build/.build/checkouts/swift-nio-http2/Sources/NIOHTTP2/HTTP2ChannelHandler.swift:195
2021-11-19T04:19:37.881177489Z 0x55a5a5c59fb8, NIOHTTP2.NIOHTTP2Handler.channelRead(context: NIOCore.ChannelHandlerContext, data: NIOCore.NIOAny) -> () at /build/.build/checkouts/swift-nio-http2/Sources/NIOHTTP2/HTTP2ChannelHandler.swift:159
2021-11-19T04:19:37.881183099Z 0x55a5a5b76c92, NIOCore.ChannelHandlerContext.(invokeChannelRead in _F5AC316541457BD146E3694279514AA3)(NIOCore.NIOAny) -> () at /build/.build/checkouts/swift-nio/Sources/NIOCore/ChannelPipeline.swift:1688
2021-11-19T04:19:37.881188748Z 0x55a5a5b7478f, NIOCore.ChannelHandlerContext.fireChannelRead(NIOCore.NIOAny) -> () at /build/.build/checkouts/swift-nio/Sources/NIOCore/ChannelPipeline.swift:1501
2021-11-19T04:19:37.882083476Z 0x55a5a5de5b39, NIOSSL.NIOSSLHandler.(doFlushReadData in _4C55B9A85907C0CB3F4E7FBD2C1C5493)(context: NIOCore.ChannelHandlerContext, receiveBuffer: NIOCore.ByteBuffer, readOnEmptyBuffer: Swift.Bool) -> () at /build/.build/checkouts/swift-nio-ssl/Sources/NIOSSL/NIOSSLHandler.swift:434
2021-11-19T04:19:37.882096276Z 0x55a5a5de5996, NIOSSL.NIOSSLHandler.(doDecodeData in _4C55B9A85907C0CB3F4E7FBD2C1C5493)(context: NIOCore.ChannelHandlerContext) -> () at /build/.build/checkouts/swift-nio-ssl/Sources/NIOSSL/NIOSSLHandler.swift:396
2021-11-19T04:19:37.882103356Z 0x55a5a5de467d, NIOSSL.NIOSSLHandler.channelRead(context: NIOCore.ChannelHandlerContext, data: NIOCore.NIOAny) -> () at /build/.build/checkouts/swift-nio-ssl/Sources/NIOSSL/NIOSSLHandler.swift:149
2021-11-19T04:19:37.882119977Z 0x55a5a5b76c92, NIOCore.ChannelHandlerContext.(invokeChannelRead in _F5AC316541457BD146E3694279514AA3)(NIOCore.NIOAny) -> () at /build/.build/checkouts/swift-nio/Sources/NIOCore/ChannelPipeline.swift:1688
2021-11-19T04:19:37.882126627Z 0x55a5a5b77928, NIOCore.ChannelPipeline.(fireChannelRead0 in _F5AC316541457BD146E3694279514AA3)(NIOCore.NIOAny) -> () at /build/.build/checkouts/swift-nio/Sources/NIOCore/ChannelPipeline.swift:897
2021-11-19T04:19:37.882132577Z 0x55a5a5b77928, NIOCore.ChannelPipeline.SynchronousOperations.fireChannelRead(NIOCore.NIOAny) -> () at /build/.build/checkouts/swift-nio/Sources/NIOCore/ChannelPipeline.swift:1158
2021-11-19T04:19:37.882274302Z 0x55a5a5cb7742, NIOPosix.BaseStreamSocketChannel.readFromSocket() throws -> NIOPosix.BaseSocketChannel<A>.ReadResult at /build/.build/checkouts/swift-nio/Sources/NIOPosix/BaseStreamSocketChannel.swift:122
2021-11-19T04:19:37.882287352Z 0x55a5a5ce70e1
2021-11-19T04:19:37.883767477Z 0x55a5a5cec903, generic specialization <NIOPosix.Socket> of NIOPosix.BaseSocketChannel.readable() -> () at .build/checkouts/swift-nio/Sources/NIOPosix/BaseSocketChannel.swift:1045
2021-11-19T04:19:37.883783368Z 0x55a5a5cec903, generic specialization <NIOPosix.Socket> of protocol witness for NIOPosix.SelectableChannel.readable() -> () in conformance NIOPosix.BaseSocketChannel<A> : NIOPosix.SelectableChannel in NIOPosix at /build/<compiler-generated>:1042
2021-11-19T04:19:37.883790608Z 0x55a5a5cec903, function signature specialization <Arg[2] = Dead> of generic specialization <NIOPosix.SocketChannel> of NIOPosix.SelectableEventLoop.handleEvent<A where A: NIOPosix.SelectableChannel>(_: NIOPosix.SelectorEventSet, channel: A) -> () at /build/.build/checkouts/swift-nio/Sources/NIOPosix/SelectableEventLoop.swift:380
2021-11-19T04:19:37.883797658Z 0x55a5a5cec452, reabstraction thunk helper from @callee_guaranteed (@guaranteed NIOPosix.SelectorEvent<NIOPosix.NIORegistration>) -> (@error @owned Swift.Error) to @escaping @callee_guaranteed (@in_guaranteed NIOPosix.SelectorEvent<NIOPosix.NIORegistration>) -> (@error @owned Swift.Error) at /build/<compiler-generated>:0
2021-11-19T04:19:37.883804838Z 0x55a5a5cec452, partial apply forwarder for reabstraction thunk helper from @callee_guaranteed (@guaranteed NIOPosix.SelectorEvent<NIOPosix.NIORegistration>) -> (@error @owned Swift.Error) to @escaping @callee_guaranteed (@in_guaranteed NIOPosix.SelectorEvent<NIOPosix.NIORegistration>) -> (@error @owned Swift.Error) at /build/<compiler-generated>:0
2021-11-19T04:19:37.883840109Z 0x55a5a5ce9ccc, generic specialization <NIOPosix.NIORegistration> of NIOPosix.Selector.whenReady0(strategy: NIOPosix.SelectorStrategy, onLoopBegin: () -> (), _: (NIOPosix.SelectorEvent<A>) throws -> ()) throws -> () at .build/checkouts/swift-nio/Sources/NIOPosix/SelectorEpoll.swift:252
2021-11-19T04:19:37.883847789Z 0x55a5a5ce7690, generic specialization <NIOPosix.NIORegistration> of NIOPosix.Selector.whenReady(strategy: NIOPosix.SelectorStrategy, onLoopBegin: () -> (), _: (NIOPosix.SelectorEvent<A>) throws -> ()) throws -> () at .build/checkouts/swift-nio/Sources/NIOPosix/SelectorGeneric.swift:286
2021-11-19T04:19:37.883855639Z 0x55a5a5ce7690, closure #2 () throws -> () in NIOPosix.SelectableEventLoop.run() throws -> () at /build/.build/checkouts/swift-nio/Sources/NIOPosix/SelectableEventLoop.swift:447
2021-11-19T04:19:37.8838692Z 0x55a5a5ce7690, reabstraction thunk helper from @callee_guaranteed () -> (@error @owned Swift.Error) to @escaping @callee_guaranteed () -> (@out (), @error @owned Swift.Error) at /build/<compiler-generated>:0
2021-11-19T04:19:37.88387587Z 0x55a5a5ce7690, generic specialization <()> of NIOPosix.withAutoReleasePool<A>(() throws -> A) throws -> A at /build/.build/checkouts/swift-nio/Sources/NIOPosix/SelectableEventLoop.swift:28
2021-11-19T04:19:37.883882241Z 0x55a5a5ce7690, NIOPosix.SelectableEventLoop.run() throws -> () at /build/.build/checkouts/swift-nio/Sources/NIOPosix/SelectableEventLoop.swift:446
2021-11-19T04:19:37.884067586Z 0x55a5a5cd2727, static NIOPosix.MultiThreadedEventLoopGroup.(runTheLoop in _C2B1528F4FBA68A3DBFA89DBAEBE9D4D)(thread: NIOPosix.NIOThread, canEventLoopBeShutdownIndividually: Swift.Bool, selectorFactory: () throws -> NIOPosix.Selector<NIOPosix.NIORegistration>, initializer: (NIOPosix.NIOThread) -> (), _: (NIOPosix.SelectableEventLoop) -> ()) -> () at /build/.build/checkouts/swift-nio/Sources/NIOPosix/MultiThreadedEventLoopGroup.swift:87
2021-11-19T04:19:37.884085466Z 0x55a5a5cd2727, closure #1 (NIOPosix.NIOThread) -> () in static NIOPosix.MultiThreadedEventLoopGroup.(setupThreadAndEventLoop in _C2B1528F4FBA68A3DBFA89DBAEBE9D4D)(name: Swift.String, selectorFactory: () throws -> NIOPosix.Selector<NIOPosix.NIORegistration>, initializer: (NIOPosix.NIOThread) -> ()) -> NIOPosix.SelectableEventLoop at /build/.build/checkouts/swift-nio/Sources/NIOPosix/MultiThreadedEventLoopGroup.swift:107
2021-11-19T04:19:37.884093897Z 0x55a5a5cd4e55, partial apply forwarder for closure #1 (NIOPosix.NIOThread) -> () in static NIOPosix.MultiThreadedEventLoopGroup.(setupThreadAndEventLoop in _C2B1528F4FBA68A3DBFA89DBAEBE9D4D)(name: Swift.String, selectorFactory: () throws -> NIOPosix.Selector<NIOPosix.NIORegistration>, initializer: (NIOPosix.NIOThread) -> ()) -> NIOPosix.SelectableEventLoop at /build/<compiler-generated>:0
2021-11-19T04:19:37.884454028Z 0x55a5a5cd5149, reabstraction thunk helper from @escaping @callee_guaranteed (@guaranteed NIOPosix.NIOThread) -> () to @escaping @callee_guaranteed (@in_guaranteed NIOPosix.NIOThread) -> (@out ()) at /build/<compiler-generated>:0
2021-11-19T04:19:37.884477238Z 0x55a5a5cd4e6d, partial apply forwarder for reabstraction thunk helper from @escaping @callee_guaranteed (@guaranteed NIOPosix.NIOThread) -> () to @escaping @callee_guaranteed (@in_guaranteed NIOPosix.NIOThread) -> (@out ()) at /build/<compiler-generated>:0
2021-11-19T04:19:37.884483699Z 0x55a5a5d008e5, closure #1 (Swift.Optional<Swift.UnsafeMutableRawPointer>) -> Swift.Optional<Swift.UnsafeMutableRawPointer> in static NIOPosix.ThreadOpsPosix.run(handle: inout Swift.Optional<Swift.UInt>, args: NIOPosix.Box<(body: (NIOPosix.NIOThread) -> (), name: Swift.Optional<Swift.String>)>, detachThread: Swift.Bool) -> () at /build/.build/checkouts/swift-nio/Sources/NIOPosix/ThreadPosix.swift:105
2021-11-19T04:19:37.884498429Z 0x7f702ab01608
2021-11-19T04:19:37.884504439Z 0x7f7029f22292
2021-11-19T04:19:37.88451598Z 0xffffffffffffffff

The problem here is: HTTP2Connections continue to emit events even after the pool has decided to shut them down, since the connections reference the pool as a let delegate. The pool must be resilient enough to ignore these events.

glbrntt
glbrntt reviewed Nov 19, 2021
View reviewed changes
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Looks good, just a comment/question.

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift
Comment on lines +523 to +524
// Connections report events back to us, if they are in a shutdown that was
// initiated by the state machine. For this reason this callback might be invoked
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't quite make sense to me. I think you're saying that we can still receive events after shutdown was initiated by the state machine, and when that happens http2Connections will be nil and we can safely ignore the event?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it'd be worth adding a comment where http2Connections is declared on L31 saying when we expect it to be nil.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also wonder if http2Connections ever needs to be nil -- presumably we can just remove all connections from it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the only reason why we made it optional was to free some memory early. I think it would make the code clearer if we make it non-optional e.g.: after we have closed a connection we set it to nil if it is empty.

async-http-client/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

Lines 543 to 545 in 12130d3

if self.http2Connections?.isEmpty == true {
self.http2Connections = nil
}

Instead we could just check if it is empty in all places where we currently check that it is nil e.g. during shutdown and make http2Connections non-optional.

The same is true in the HTTP2StateMachine for http1Connections.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dnadoba will fix this in a follow up PR.

dnadoba
dnadoba approved these changes Nov 19, 2021
View reviewed changes
Copy link
Collaborator

@dnadoba dnadoba left a comment

Choose a reason for hiding this comment

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

This should fix the issue, thanks! New test cases look also good for HTTP2Connections and HTTP2StateMachine. Just one nit: maybe we should also add one test case for the HTTP1StateMachine.

@fabianfett fabianfett merged commit 2fe3f42 into swift-server:main Nov 19, 2021
@dnadoba dnadoba mentioned this pull request Nov 19, 2021
Closed
@fabianfett fabianfett added the 🔨 semver/patch No public API change. label Nov 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glbrntt glbrntt glbrntt left review comments

@dnadoba dnadoba dnadoba approved these changes

Assignees
No one assigned
Labels
🔨 semver/patch No public API change.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fabianfett @glbrntt @dnadoba