-
Notifications
You must be signed in to change notification settings - Fork 276
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
deferred responses should be compressed #1572
Comments
Here's how I added the delay. And the curl:
|
thanks, I can reproduce it |
ok, the issue is with response compression. If it is not compressed then the chunks come at the right time |
Fix #1572 async-compression is used in tower-http's CompressionLayer. Inside the AsyncRead based compression code, if the underlying stream returns Poll::Pending, it will be returned directly, while in the case of deferred responses, the next one might come way later, so we want to send whatever data we have available right now. We will have to switch to an AsyncWrite interface instead, which will be more flexible, but considering the timing of the release, this patch will hold for now. This uses the code from Nullus157/async-compression#155
We have a work-around for 1.0 but we’d like to switch to a proper solution which is being designed in Nullus157/async-compression#154. Reopening to track that. |
…esponses instead This is a different work-around for #1572. The previous work-around required a `[patch.crates-io]` section in the root `Cargo.toml`. When the Router is used as a dependency, that root manifest is not ours so downstream users would need to apply the same patch to get the work-around. This came up in the context of publish the Router to crates.io #491 but applies the same when the `apollo-router` crate is used as a git dependency.
…esponses instead This is a different work-around for #1572. The previous work-around required a `[patch.crates-io]` section in the root `Cargo.toml`. When the Router is used as a dependency, that root manifest is not ours so downstream users would need to apply the same patch to get the work-around. This came up in the context of publish the Router to crates.io #491 but applies the same when the `apollo-router` crate is used as a git dependency.
…esponses instead This is a different work-around for #1572. The previous work-around required a `[patch.crates-io]` section in the root `Cargo.toml`. When the Router is used as a dependency, that root manifest is not ours so downstream users would need to apply the same patch to get the work-around. This came up in the context of publish the Router to crates.io #491 but applies the same when the `apollo-router` crate is used as a git dependency.
…esponses instead This is a different work-around for #1572. The previous work-around required a `[patch.crates-io]` section in the root `Cargo.toml`. When the Router is used as a dependency, that root manifest is not ours so downstream users would need to apply the same patch to get the work-around. This came up in the context of publish the Router to crates.io #491 but applies the same when the `apollo-router` crate is used as a git dependency.
…esponses instead This is a different work-around for #1572. The previous work-around required a `[patch.crates-io]` section in the root `Cargo.toml`. When the Router is used as a dependency, that root manifest is not ours so downstream users would need to apply the same patch to get the work-around. This came up in the context of publish the Router to crates.io #491 but applies the same when the `apollo-router` crate is used as a git dependency.
…esponses instead This is a different work-around for #1572. The previous work-around required a `[patch.crates-io]` section in the root `Cargo.toml`. When the Router is used as a dependency, that root manifest is not ours so downstream users would need to apply the same patch to get the work-around. This came up in the context of publish the Router to crates.io #491 but applies the same when the `apollo-router` crate is used as a git dependency.
…esponses instead This is a different work-around for #1572. The previous work-around required a `[patch.crates-io]` section in the root `Cargo.toml`. When the Router is used as a dependency, that root manifest is not ours so downstream users would need to apply the same patch to get the work-around. This came up in the context of publish the Router to crates.io #491 but applies the same when the `apollo-router` crate is used as a git dependency.
I've renamed this issue to reflect the state of the work-around, but we should use the original reproduction above to verify any eventual fix. |
I believe 67aae13 provides automated test coverage for this, but if we have another reproduction it’d be good to check as well. |
follow up work is in Nullus157/async-compression#178 |
I'll do that as a custom layer in the router instead |
reported by @BoD
with the latest version of the supergraph demo with defer, I’ve added an artificial 4s delay in the Inventory subgraph responses. Then making a query like this:
I was expecting to receive the initial payload immediately and the 2nd one after 4s, but I’m receiving everything at once after 4s.
The text was updated successfully, but these errors were encountered: