-
Notifications
You must be signed in to change notification settings - Fork 125
FileDownloadDelegate can issue out-of-order writes #348
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
Comments
@Lukasa The delegate must already guarantee that only one of them is in flight at a time, or else this is completely unusable. ie. I agree. We do have an example in NIO that demonstrates how to write a file downloader with backpressure etc but just like @Lukasa suggests we really have to fix the delegate call-out code, or else we're setting all of our users up for failure. There should only ever be one delegate call-out in progress at the same time and for delegate methods that return futures "in progress" means from the call until the future is fulfilled. |
Motivation: Users of the HTTPClientResponseDelegate expect that the event loop futures returned from didReceiveHead and didReceiveBodyPart can be used to exert backpressure. To be fair to them, they somewhat can. However, the TaskHandler has a bit of a misunderstanding about how NIO backpressure works, and does not correctly manage the buffer of inbound data. The result of this misunderstanding is that multiple calls to didReceiveBodyPart and didReceiveHead can be outstanding at once. This would likely lead to severe bugs in most delegates, as they do not expect it. We should make things work the way delegate implementers believe it works. Modifications: - Added a buffer to the TaskHandler to avoid delivering data that the delegate is not ready for. - Added a new "pending close" state that keeps track of a state where the TaskHandler has received .end but not yet delivered it to the delegate. This allows better error management. - Added some more tests. - Documented our backpressure commitments. Result: Better respect for backpressure. Resolves swift-server#348
Motivation: Users of the HTTPClientResponseDelegate expect that the event loop futures returned from didReceiveHead and didReceiveBodyPart can be used to exert backpressure. To be fair to them, they somewhat can. However, the TaskHandler has a bit of a misunderstanding about how NIO backpressure works, and does not correctly manage the buffer of inbound data. The result of this misunderstanding is that multiple calls to didReceiveBodyPart and didReceiveHead can be outstanding at once. This would likely lead to severe bugs in most delegates, as they do not expect it. We should make things work the way delegate implementers believe it works. Modifications: - Added a buffer to the TaskHandler to avoid delivering data that the delegate is not ready for. - Added a new "pending close" state that keeps track of a state where the TaskHandler has received .end but not yet delivered it to the delegate. This allows better error management. - Added some more tests. - Documented our backpressure commitments. Result: Better respect for backpressure. Resolves swift-server#348
Motivation: Users of the HTTPClientResponseDelegate expect that the event loop futures returned from didReceiveHead and didReceiveBodyPart can be used to exert backpressure. To be fair to them, they somewhat can. However, the TaskHandler has a bit of a misunderstanding about how NIO backpressure works, and does not correctly manage the buffer of inbound data. The result of this misunderstanding is that multiple calls to didReceiveBodyPart and didReceiveHead can be outstanding at once. This would likely lead to severe bugs in most delegates, as they do not expect it. We should make things work the way delegate implementers believe it works. Modifications: - Added a buffer to the TaskHandler to avoid delivering data that the delegate is not ready for. - Added a new "pending close" state that keeps track of a state where the TaskHandler has received .end but not yet delivered it to the delegate. This allows better error management. - Added some more tests. - Documented our backpressure commitments. Result: Better respect for backpressure. Resolves swift-server#348
Motivation: Users of the HTTPClientResponseDelegate expect that the event loop futures returned from didReceiveHead and didReceiveBodyPart can be used to exert backpressure. To be fair to them, they somewhat can. However, the TaskHandler has a bit of a misunderstanding about how NIO backpressure works, and does not correctly manage the buffer of inbound data. The result of this misunderstanding is that multiple calls to didReceiveBodyPart and didReceiveHead can be outstanding at once. This would likely lead to severe bugs in most delegates, as they do not expect it. We should make things work the way delegate implementers believe it works. Modifications: - Added a buffer to the TaskHandler to avoid delivering data that the delegate is not ready for. - Added a new "pending close" state that keeps track of a state where the TaskHandler has received .end but not yet delivered it to the delegate. This allows better error management. - Added some more tests. - Documented our backpressure commitments. Result: Better respect for backpressure. Resolves swift-server#348
Motivation: Users of the HTTPClientResponseDelegate expect that the event loop futures returned from didReceiveHead and didReceiveBodyPart can be used to exert backpressure. To be fair to them, they somewhat can. However, the TaskHandler has a bit of a misunderstanding about how NIO backpressure works, and does not correctly manage the buffer of inbound data. The result of this misunderstanding is that multiple calls to didReceiveBodyPart and didReceiveHead can be outstanding at once. This would likely lead to severe bugs in most delegates, as they do not expect it. We should make things work the way delegate implementers believe it works. Modifications: - Added a buffer to the TaskHandler to avoid delivering data that the delegate is not ready for. - Added a new "pending close" state that keeps track of a state where the TaskHandler has received .end but not yet delivered it to the delegate. This allows better error management. - Added some more tests. - Documented our backpressure commitments. Result: Better respect for backpressure. Resolves #348
FileDownloadDelegate
writes file body parts to the filesystem in the following code:async-http-client/Sources/AsyncHTTPClient/FileDownloadDelegate.swift
Lines 83 to 102 in 0dda95c
This code incorrectly assumes that only one write may be in flight at a given time. While the delegate protocol does exert backpressure, it is entirely possible for multiple
.body
chunks to be triggered on a singlechannelRead
, particularly in the case of chunked transfer encoding. The delegate protocol does not protect against this and currently does not promise that this will not happen.We should either fix the delegate, or fix the delegate protocol to guarantee that only one call to
didReceiveHead
,didReceiveBodyPart
, anddidFinishRequest
are in flight at any given time. I'm inclined to want to clean up our use of the delegate to make it easier to program against, rather than to just fix the downloader, but I'd like to hear other opinions on the matter, particularly from @weissi and @artemredkin.The text was updated successfully, but these errors were encountered: