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

deferred responses should be compressed #1572

Closed
Tracked by #80
Geal opened this issue Aug 23, 2022 · 8 comments · Fixed by #1604 or #2986
Closed
Tracked by #80

deferred responses should be compressed #1572

Geal opened this issue Aug 23, 2022 · 8 comments · Fixed by #1604 or #2986
Assignees
Labels
component/defer enhancement An enhancement to an existing feature

Comments

@Geal
Copy link
Contributor

Geal commented Aug 23, 2022

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:

query Product {
  product(id: "apollo-federation") {
    # Comes from Products subgraph (fast)
    dimensions {
      size
    }

    # Comes from Inventory subgraph (slow)
    ... on Product @defer {
      delivery {
        estimatedDelivery
        fastestDelivery
      }
    }
  }
} 

I was expecting to receive the initial payload immediately and the 2nd one after 4s, but I’m receiving everything at once after 4s.

@BoD
Copy link

BoD commented Aug 23, 2022

Here's how I added the delay.

And the curl:

curl 'http://localhost:4000/'   -H 'content-type: application/json'   --data-raw $'{"query":"query Product($productId: ID!) {product(id: $productId) {dimensions {size} ... on Product @defer {delivery { estimatedDelivery fastestDelivery } } } }","variables":{"productId":"apollo-federation"},"operationName":"Product"}' --compressed

@Geal
Copy link
Contributor Author

Geal commented Aug 23, 2022

thanks, I can reproduce it

@Geal
Copy link
Contributor Author

Geal commented Aug 23, 2022

ok, the issue is with response compression. If it is not compressed then the chunks come at the right time

@abernix abernix added this to the v1.0.0-alpha.0 milestone Aug 23, 2022
Geal added a commit that referenced this issue Aug 26, 2022
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
@abernix abernix mentioned this issue Sep 2, 2022
25 tasks
@SimonSapin
Copy link
Contributor

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.

@SimonSapin SimonSapin reopened this Sep 12, 2022
@SimonSapin SimonSapin modified the milestones: v1.0.0-alpha.0, post-v1.0 Sep 12, 2022
SimonSapin added a commit that referenced this issue Sep 12, 2022
…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.
SimonSapin added a commit that referenced this issue Sep 12, 2022
…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.
SimonSapin added a commit that referenced this issue Sep 12, 2022
…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.
SimonSapin added a commit that referenced this issue Sep 12, 2022
…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.
SimonSapin added a commit that referenced this issue Sep 12, 2022
…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.
SimonSapin added a commit that referenced this issue Sep 12, 2022
…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.
SimonSapin added a commit that referenced this issue Sep 12, 2022
…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.
@abernix abernix changed the title primary response sent at the same time as deferred response deferred responses should be compressed Sep 14, 2022
@abernix
Copy link
Member

abernix commented Sep 14, 2022

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.

@abernix abernix added the enhancement An enhancement to an existing feature label Sep 14, 2022
@SimonSapin
Copy link
Contributor

I believe 67aae13 provides automated test coverage for this, but if we have another reproduction it’d be good to check as well.

@abernix abernix removed this from the v1.x.x milestone Oct 17, 2022
@Geal
Copy link
Contributor Author

Geal commented Oct 17, 2022

follow up work is in Nullus157/async-compression#178

@Geal
Copy link
Contributor Author

Geal commented Feb 22, 2023

I'll do that as a custom layer in the router instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/defer enhancement An enhancement to an existing feature
Projects
None yet
4 participants