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

Execute the entire request pipeline if the client closed the connection #4770

Merged
merged 17 commits into from
Mar 26, 2024

Conversation

Geal
Copy link
Contributor

@Geal Geal commented Mar 8, 2024

Fix #4569
Fix #4576
Fix #4589
Fix #4590
Fix #4611

When the client closes the connection prematurely, it drops the entire request handling task, which means that it won't go through the entire response pipeline, where we record the operation and handle telemetry. Some users also have additional steps with rhai or coprocessors where they add metadata, and those steps should run even on canceled requests. This moves the request handling to a separate task to make sure it runs, but it also skips subgraph requests if we detected that the client closed the connection, to prevent unneeded traffic.

Description here

Fixes #issue_number


Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

  • Changes are compatible1
  • Documentation2 completed
  • Performance impact assessed and acceptable
  • Tests added and passing3
    • Unit Tests
    • Integration Tests
    • Manual Tests

Exceptions

Note any exceptions here

Notes

Footnotes

  1. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this.

  2. Configuration is an important part of many changes. Where applicable please try to document configuration examples.

  3. Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions.

When the client closes the connection prematurely, it drops the entire
request handling task, which means that it won't go through the entire
response pipeline, where we record the operation and handle telemetry.
Some users also have additional steps with rhai or coprocessors where
they add metadata, and those steps should run even on canceled requests.
This moves the request handling to a separate task to make sure it runs,
but it also skips subgraph requests if we detected that the client
closed the connection, to prevent unneeded traffic.

This comment has been minimized.

@router-perf
Copy link

router-perf bot commented Mar 8, 2024

CI performance tests

  • reload - Reload test over a long period of time at a constant rate of users
  • events_big_cap_high_rate_callback - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity using callback mode
  • events_without_dedup_callback - Stress test for events with a lot of users and deduplication DISABLED using callback mode
  • large-request - Stress test with a 1 MB request payload
  • const - Basic stress test that runs with a constant number of users
  • no-graphos - Basic stress test, no GraphOS.
  • step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
  • events - Stress test for events with a lot of users and deduplication ENABLED
  • events_callback - Stress test for events with a lot of users and deduplication ENABLED in callback mode
  • events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity
  • events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
  • xxlarge-request - Stress test with 100 MB request payload
  • xlarge-request - Stress test with 10 MB request payload
  • step - Basic stress test that steps up the number of users over time

@Geal Geal force-pushed the geal/request-completion branch from 0a979c6 to fe91463 Compare March 8, 2024 17:35
@Geal Geal marked this pull request as ready for review March 12, 2024 09:58
@Geal Geal requested a review from a team March 12, 2024 10:05
Copy link
Contributor

@o0Ignition0o o0Ignition0o left a comment

Choose a reason for hiding this comment

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

LGTM

apollo-router/src/services/router/service.rs Show resolved Hide resolved
@Geal Geal requested a review from a team as a code owner March 25, 2024 13:20
@Geal Geal enabled auto-merge (squash) March 25, 2024 13:21
@Geal Geal merged commit 85296cc into dev Mar 26, 2024
13 checks passed
@Geal Geal deleted the geal/request-completion branch March 26, 2024 09:59
BrynCooke pushed a commit that referenced this pull request Mar 27, 2024
#4770 introduced a log message when a broken pipe is detected. This PR makes this log message opt-in.

Users that have internet facing routers will likely not want to opt in to this log message as they have no control over the clients.
@BrynCooke BrynCooke mentioned this pull request Mar 27, 2024
6 tasks
BrynCooke pushed a commit that referenced this pull request Mar 27, 2024
#4770 introduced a log message when a broken pipe is detected. This PR makes this log message opt-in.

Users that have internet facing routers will likely not want to opt in to this log message as they have no control over the clients.
BrynCooke added a commit that referenced this pull request Mar 27, 2024
## Experimental logging of broken pipe errors

You can emit a log message each time the client closes the connection
early, which can help you debug issues with clients that close
connections before the server can respond.

This feature is disabled by default but can be enabled by setting the
`experimental_log_broken_pipe` option to `true`:

```yaml title="router.yaml"
supergraph:
  experimental_log_broken_pipe: true
```
| Attribute | Default | Description |

|----------------------------------|---------|-----------------------------------------------------|
| `experimental_log_broken_pipe` | false | Emit a log message if a
broken pipe was detected. |



#4770 introduced a log message when a broken pipe is detected. This PR
makes this log message opt-in.

Users that have internet facing routers will likely not want to opt in
to this log message as they have no control over the clients.


<!-- start metadata -->
---

**Checklist**

Complete the checklist (and note appropriate exceptions) before the PR
is marked ready-for-review.

- [ ] Changes are compatible[^1]
- [ ] Documentation[^2] completed
- [ ] Performance impact assessed and acceptable
- Tests added and passing[^3]
    - [ ] Unit Tests
    - [ ] Integration Tests
    - [ ] Manual Tests

**Exceptions**

*Note any exceptions here*

**Notes**

[^1]: It may be appropriate to bring upcoming changes to the attention
of other (impacted) groups. Please endeavour to do this before seeking
PR approval. The mechanism for doing this will vary considerably, so use
your judgement as to how and when to do this.
[^2]: Configuration is an important part of many changes. Where
applicable please try to document configuration examples.
[^3]: Tick whichever testing boxes are applicable. If you are adding
Manual Tests, please document the manual testing (extensively) in the
Exceptions.

---------

Co-authored-by: bryn <bryn@apollographql.com>
@BrynCooke BrynCooke mentioned this pull request Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment