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 HEAD not being retried for HTTP 500 category responses #581

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nh2
Copy link
Contributor

@nh2 nh2 commented Apr 14, 2023

Tentative PR to help with #579.

This fixes the immediate issue of key HTTP 500 errors such as 502 Bad Gateway not leading to retries.

But as described in #579 (comment) I thikn that even more cases should lead to retry.

Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

Thank you! This should fix the problem. Could you please add a case for our test suite?

lib/upload.js Outdated
@@ -615,6 +615,11 @@ class BaseUpload {
if (status === 423) {
this._emitHttpError(req, res, 'tus: upload is currently locked; retry later')
return
} else if (inStatusCategory(status, 500)) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's not use an else if here but just an if. The else should not be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -615,6 +615,11 @@ class BaseUpload {
if (status === 423) {
this._emitHttpError(req, res, 'tus: upload is currently locked; retry later')
return
} else if (inStatusCategory(status, 500)) {
// Run retry logic if the server has an error, e.g. 502 Bad Gateway when
// proxied server is temporarily down. See issue #579.
Copy link
Member

Choose a reason for hiding this comment

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

Please mention that we do not want to clear this._url and create a new upload here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@nh2 nh2 force-pushed the issue-579-retry-HEAD-on-intermittent-HTTP-errors branch from d257b9b to b4e3693 Compare April 15, 2023 20:45
@Acconut
Copy link
Member

Acconut commented Apr 16, 2023

Thanks for the updates. Only a test case would be nice now:

Could you please add a case for our test suite?

Let me know if you need help with that.

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