Skip to content

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
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.

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