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.

Sign up for GitHub

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

Jump to bottom

Apply timeout middleware to all routes #148

Closed
MadLittleMods opened this issue Dec 1, 2022 · 0 comments · Fixed by #204
Closed

Apply timeout middleware to all routes #148

MadLittleMods opened this issue Dec 1, 2022 · 0 comments · Fixed by #204
Labels
A-backend Related to the backend Node.js server pieces T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Milestone
Matrix Public Arc...

Comments

@MadLittleMods
Copy link
Contributor

MadLittleMods commented Dec 1, 2022 •
edited

Move the timeout-middleware to apply to all routes.

It would also be good to send a metric whenever we see a request timeout, related to #45

Adjacent: Here is an example middleware from the Gitter webapp that logs and metrics when a request is pending for more than 60 seconds, https://gitlab.com/gitterHQ/webapp/-/blob/676fadc3693260c8c51f448a0ca4c3e180d1b4a2/server/web/middlewares/pending-request.js#L50-84
@MadLittleMods MadLittleMods added the T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. label Dec 1, 2022
@MadLittleMods MadLittleMods added the A-backend Related to the backend Node.js server pieces label Apr 11, 2023
@MadLittleMods MadLittleMods mentioned this issue Apr 19, 2023
Merged
MadLittleMods added a commit that referenced this issue Apr 19, 2023
@MadLittleMods
Add route identifiers for easy metric reporting (#173)
78ee88e 
Pre-requisite for #162 and #148
MadLittleMods added a commit that referenced this issue May 1, 2023
@MadLittleMods
Timeout requests and stop processing further
e717b41 
Fix #148, #40
@MadLittleMods MadLittleMods mentioned this issue May 1, 2023
Merged
2 tasks
MadLittleMods added a commit that referenced this issue May 2, 2023
@MadLittleMods
Timeout requests and stop processing further (#204)
9078abf 
Fix #148
Fix #40

 - Apply timeout middleware to all room directory and room routes
 - Stop messing with the response after we timeout. Fix #148
    - This also involves cancelling any `async/await` things like requests in the routes so we throw an abort error instead of continuing on. Fix #40
 - Also abort the route if we see that the user closed the request before we could respond to them
 - Bumps minimum supported Node.js version to v18 because we're now using the built-in native `fetch` in Node.js vs `node-fetch`. This gives us the custom `signal.reason` that we aborted with instead of a generic `AbortError`.
    - This also means we had to add some instrumentation for `fetch` which uses `undici` under the hood. Settled on some unofficial instrumentation: [`opentelemetry-instrumentation-fetch-node`](https://www.npmjs.com/package/opentelemetry-instrumentation-fetch-node)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-backend Related to the backend Node.js server pieces T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
Milestone
Matrix Public Archive v0.1.0
Development

Successfully merging a pull request may close this issue.

Timeout requests and stop processing further matrix-org/matrix-viewer
1 participant
@MadLittleMods