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

fix(api): fix media route timeout #1170

Merged
merged 2 commits into from
Mar 7, 2024
Merged

fix(api): fix media route timeout #1170

merged 2 commits into from
Mar 7, 2024

Conversation

kishore03109
Copy link
Contributor

@kishore03109 kishore03109 commented Feb 26, 2024

Problem

We had an issue where a request such as "media/images%20blah/pages" timeing out, see incident here. while we do have an overall catch all in our main router, there are quite a number of calls in our subrouter that fails.

Solution

for the media subrouter, this should return a 404. this is not applied across all the routes as some of the routes depend on the fall through behaviour to function. therefore, this pr is only scoped to return only for the media subrouter

Other solutions that did not work

  1. adding a next() to every subrouter
  2. adding an error handler to every subrouter such that if subrouter captures a group and does not handle it, an error should be thrown. unfortunately, this caused quite a number of functionality to break, rather this hotfix go in first while and revisit this when we get timeouts again

Breaking Changes

  • Yes - this PR contains breaking changes
    • Details ...
  • No - this PR is backwards compatible with ALL of the following feature flags in this doc

Tests

  • Get call an authenticated request (ie you have assess to the site with the relevant valid cookie) to the backend
GET http://localhost:8081/v2/sites/kishore-test-dev-gh/media/images%2Fblah.png/pages
  • Assert that you receive a res not found error rather than a timeout
Screenshot 2024-03-04 at 8 21 16 AM

@kishore03109 kishore03109 changed the title temp commit fix(api): prevernt server crashes Feb 26, 2024
@kishore03109 kishore03109 marked this pull request as ready for review February 27, 2024 09:17
@kishore03109 kishore03109 requested review from a team and timotheeg February 27, 2024 09:17
@kishore03109 kishore03109 marked this pull request as draft February 28, 2024 03:54
@kishore03109 kishore03109 marked this pull request as ready for review March 4, 2024 01:33
@kishore03109 kishore03109 changed the title fix(api): prevernt server crashes fix(api): fix media route timeout Mar 4, 2024
src/routes/v2/authenticatedSites/media.ts Outdated Show resolved Hide resolved
Comment on lines +450 to +451
router.use(catchNonExistentRoutesMiddleware)

Copy link
Contributor

Choose a reason for hiding this comment

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

why not put this as the terminating middleware in our server.js? this would allow us to catch all missing routes and to return a sensible response

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a terminating middleware already exists at server.js where our overall error handler resides.

the problem exists when a subrouter captures a group and does not handle it, causing a timeout.
do note that some of our routes fall through while others dont, so i couldnt append this at the end of all subrouters because that broke quite a number of functionality within the cms, and the manual test space for such a pr will be unreasonably large. stance here will be to monitor for 5xx errors and then fix them

Copy link
Contributor

Choose a reason for hiding this comment

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

actually that's weird because i always thought if the subrouter doesn't capture the route, it should fall over to the next one.

ie, if subrouter A captures /about/a and i get a request with /about/b, it should fall to the next router in the stack.

this isn't very important given that we assume most users are calling from the frontend but it is weird tho

@kishore03109 kishore03109 requested a review from seaerchin March 5, 2024 07:30
@kishore03109 kishore03109 merged commit 12c22bc into develop Mar 7, 2024
9 checks passed
@mergify mergify bot deleted the fix/5xxErrors branch March 7, 2024 07:09
@alexanderleegs alexanderleegs mentioned this pull request Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants