-
Notifications
You must be signed in to change notification settings - Fork 316
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
d257b9b
to
b4e3693
Compare
Thanks for the updates. Only a test case would be nice now:
Let me know if you need help with that. |
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.