Fix a connection reuse race #191
Conversation
patrickbkr
force-pushed
the
http11-retry
branch
from
a542aff to
77969a9
Compare
patrickbkr
changed the title
Connection reuse is subject to a race condition where remote connection tear down races with reusing the connection for a new request. There were already locks in place for access to `$!next-response-vow` / `%!outstanding-stream-responses` but the lock didn't cover the check for `$!dead`, thus it was possible for the dead check to succeed, the connection being torn down and only then the next response vow to be taken.
| $!next-response-vow = $next-response-promise.vow; | ||
| } | ||
| } | ||
| return Promise.broken(PipelineClosedBeforeHeaders.new) if $broken; |
There was a problem hiding this comment.
Maybe this line would be better in the if $!dead arm and no need for var $broken
There was a problem hiding this comment.
Yes it would be better. But see the comment. ;-)
There was a problem hiding this comment.
To elaborate a bit more, the PR referenced in the comment actually has been merged. But I don't intentionally want to break Cro for Rakudo versions <= 2024.05 yet.
There was a problem hiding this comment.
oh - yes I see the comment is related - missed that!
| } | ||
| return Promise.broken(PipelineClosedBeforeHeaders.new) if $broken; |
There was a problem hiding this comment.
I have reviewed for code quality and ensured that there is no malicious code
My experience with concurrency is limited - so I am not able to confirm that eg. any source of deadlock with other interacting code has been identified
So this review is caveat that tests over a range of asynchronous situations have been performed.
HTTP1.1 connection reuse is subject to a race condition where remote connection tear down races with reusing the connection for a new request. This change wires HTTP1.1 into the same request retry mechanism HTTP2 uses.
HTTP2 was subject to the same race. I updated the PR respectively.