Skip to content

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

Closed
Lukasa opened this issue Mar 16, 2021 · 1 comment · Fixed by #352
Closed

FileDownloadDelegate can issue out-of-order writes #348

Lukasa opened this issue Mar 16, 2021 · 1 comment · Fixed by #352
Labels
kind/bug Feature doesn't work as expected.

Comments

@Lukasa
Copy link
Collaborator

Lukasa commented Mar 16, 2021

FileDownloadDelegate writes file body parts to the filesystem in the following code:

let writeFuture: EventLoopFuture<Void>
if let fileHandleFuture = self.fileHandleFuture {
writeFuture = fileHandleFuture.flatMap {
self.io.write(fileHandle: $0, buffer: buffer, eventLoop: task.eventLoop)
}
} else {
let fileHandleFuture = self.io.openFile(
path: self.filePath,
mode: .write,
flags: .allowFileCreation(),
eventLoop: task.eventLoop
)
self.fileHandleFuture = fileHandleFuture
writeFuture = fileHandleFuture.flatMap {
self.io.write(fileHandle: $0, buffer: buffer, eventLoop: task.eventLoop)
}
}
self.writeFuture = writeFuture
return writeFuture

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 single channelRead, 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, and didFinishRequest 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.

@Lukasa Lukasa added the kind/bug Feature doesn't work as expected. label Mar 16, 2021
@weissi
Copy link
Contributor

weissi commented Mar 16, 2021

@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.

Lukasa added a commit to Lukasa/async-http-client that referenced this issue Mar 29, 2021
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
Lukasa added a commit to Lukasa/async-http-client that referenced this issue Mar 30, 2021
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
Lukasa added a commit to Lukasa/async-http-client that referenced this issue Mar 30, 2021
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
Lukasa added a commit to Lukasa/async-http-client that referenced this issue Mar 30, 2021
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
Lukasa added a commit that referenced this issue Mar 30, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Feature doesn't work as expected.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants