Skip to content
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

🕳 Implement finish semantics for streaming bodies #203

Merged
merged 4 commits into from
Jan 18, 2023

Conversation

acfoltzer
Copy link
Contributor

@acfoltzer acfoltzer commented Nov 9, 2022

Moving forward, streaming bodies must be affirmatively finished when Wasm programs are finished writing to them. This helps prevent unexpected control flows due to error handling causing an incomplete transfer-encoding: chunked streaming body from appearing complete to its recipient.

These changes require updates to the integration test fixtures with a prerelease version of the SDK. Once the 0.9.0 Rust SDK is released, we'll follow up with an update to the released version.

@acfoltzer acfoltzer self-assigned this Nov 9, 2022
@acfoltzer acfoltzer added the feature-prod Related to matching the production C@E environment label Nov 9, 2022
@acfoltzer acfoltzer force-pushed the acf/finish-streaming-bodies branch from 7fbb42e to d324cb9 Compare November 14, 2022 19:57
@acfoltzer acfoltzer marked this pull request as ready for review November 14, 2022 20:02
@acfoltzer
Copy link
Contributor Author

Note that we are not ready to cut a release including these changes, so we should probably hold off on merging until then. But the changes are ready for review.

@joeshaw joeshaw mentioned this pull request Dec 1, 2022
cratelyn
cratelyn previously approved these changes Jan 6, 2023
Copy link
Contributor

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

this looks fantastic @acfoltzer, thank you for handling this tricky issue!

Moving forward, streaming bodies must be affirmatively finished when Wasm programs are finished
writing to them. This helps prevent unexpected control flows due to error handling causing an
incomplete `transfer-encoding: chunked` streaming body from appearing complete to its recipient.

These changes require updates to the integration test fixtures with a prerelease version of the
SDK. Once the 0.9.0 Rust SDK is released, we'll follow up with an update to the released version.
@acfoltzer
Copy link
Contributor Author

@cratelyn I prematurely retagged you on this... my plan is to do the 0.9.0 release, update the test fixtures' Cargo.toml appropriately, and then merge+release Viceroy.

@acfoltzer acfoltzer removed the request for review from cratelyn January 18, 2023 18:09
cratelyn
cratelyn previously approved these changes Jan 18, 2023
Copy link
Contributor

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

✔️ this looks great! i have just one tiny optional nitpick about imports, take it or leave it. thank you for doing this @acfoltzer!

lib/src/body.rs Outdated
Comment on lines 3 to 7
use crate::streaming_body::StreamingBodyItem;
use crate::Error;

use {
crate::error,
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, nit: can we squish this into the use below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 5f2d111. Do you know if there's a combination of settings that would get rust-analyzer to insert imports this way? https://rust-analyzer.github.io/manual.html#auto-import

I tend to exclusively use the auto-import feature without even looking at what it inserts, which is how I missed this inconsistency.

@acfoltzer acfoltzer requested a review from cratelyn January 18, 2023 21:09
@acfoltzer
Copy link
Contributor Author

Please hold... I will update the lockfiles once we have 0.9.1 out the door 😞

Copy link
Member

@joeshaw joeshaw left a comment

Choose a reason for hiding this comment

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

👍

@acfoltzer acfoltzer merged commit c1fafea into main Jan 18, 2023
@acfoltzer acfoltzer deleted the acf/finish-streaming-bodies branch January 18, 2023 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-prod Related to matching the production C@E environment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants