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: handle warmup request error correctly #16223

Merged

Conversation

sapphi-red
Copy link
Member

Description

Alternative to #16216

From the stack trace, I guess this would catch the error correctly.

The reason why .catch(e => cannot catch the error is because transformRequest throws an error in sync instead of async.

if (server._restartPromise && !options.ssr) throwClosedServerError()

Sync errors can only be catched by try {} catch {}.

The fix for it would be to change transformRequest to an async function (that would change the sync error into async error), or, to use try { await transformRequest() } catch {}. Using both .catch and try {} catch {} is also an option.
https://stackblitz.com/edit/node-zucmtr?file=index.js

I went with try { await transformRequest() } catch {}.

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines, especially the Pull Request Guidelines.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

@sapphi-red sapphi-red added p3-minor-bug An edge case that only affects very specific usage (priority) feat: dev dev server labels Mar 21, 2024
Copy link

stackblitz bot commented Mar 21, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev
Copy link
Member

Thanks for the alternative! I'm not sure if we should keep throwing the error though. Maybe it is ok to try first this PR. I'm afraid now about external code calling server.transformRequest and pluginContainer.resolve. We aren't exposing formally this error, and I don't think these are properly handled. It may be better to let the current processing to finish when restarting the server. If we bailout from warmup requests, the pending processing should clear rather quickly.

But I'm fine going with this one if you prefer that.

@sapphi-red
Copy link
Member Author

I'm afraid now about external code calling server.transformRequest and pluginContainer.resolve. We aren't exposing formally this error, and I don't think these are properly handled.

Makes sense. But if we are going that way, I think we'll need to remove all of these.

if (e?.code === ERR_OPTIMIZE_DEPS_PROCESSING_ERROR) {
// Skip if response has already been sent
if (!res.writableEnded) {
res.statusCode = 504 // status code request timeout
res.statusMessage = 'Optimize Deps Processing Error'
res.end()
}
// This timeout is unexpected
server.config.logger.error(e.message)
return
}
if (e?.code === ERR_OUTDATED_OPTIMIZED_DEP) {
// Skip if response has already been sent
if (!res.writableEnded) {
res.statusCode = 504 // status code request timeout
res.statusMessage = 'Outdated Optimize Dep'
res.end()
}
// We don't need to log an error in this case, the request
// is outdated because new dependencies were discovered and
// the new pre-bundle dependencies have changed.
// A full-page reload has been issued, and these old requests
// can't be properly fulfilled. This isn't an unexpected
// error but a normal part of the missing deps discovery flow
return
}
if (e?.code === ERR_CLOSED_SERVER) {
// Skip if response has already been sent
if (!res.writableEnded) {
res.statusCode = 504 // status code request timeout
res.statusMessage = 'Outdated Request'
res.end()
}
// We don't need to log an error in this case, the request
// is outdated because new dependencies were discovered and
// the new pre-bundle dependencies have changed.
// A full-page reload has been issued, and these old requests
// can't be properly fulfilled. This isn't an unexpected
// error but a normal part of the missing deps discovery flow
return
}
if (e?.code === ERR_FILE_NOT_FOUND_IN_OPTIMIZED_DEP_DIR) {
// Skip if response has already been sent
if (!res.writableEnded) {
res.statusCode = 404
res.end()
}
server.config.logger.warn(colors.yellow(e.message))
return
}
if (e?.code === ERR_LOAD_URL) {
// Let other middleware handle if we can't load the url via transformRequest
return next()
}

@patak-dev
Copy link
Member

I think we could keep the ERR_OPTIMIZE_DEPS_PROCESSING_ERROR, but ya... I see your point about ERR_OUTDATED_OPTIMIZED_DEP. Ok, let's merge this version first and evaluate our options once we have real bug reports.

@patak-dev patak-dev merged commit d7c5256 into vitejs:main Mar 21, 2024
10 checks passed
@sapphi-red sapphi-red deleted the fix/handle-warmup-request-error-correctly branch March 21, 2024 09:03
@cpojer
Copy link
Contributor

cpojer commented Mar 22, 2024

Thank you 🙇‍♂️ Looking forward to trying this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: dev dev server p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants