-
Notifications
You must be signed in to change notification settings - Fork 115
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
[Colossus] fix: upload aborted error #5094
[Colossus] fix: upload aborted error #5094
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.
Great that you found this settings. I suppose we should have looked more closely at changes between v14 and v18 that would have impacted our applications. Let keep that in mind when we shift to v20+
If we want a quick deployment of this fix because we are seeing many failed uploads this is good to go. Just need to bump colossus version.
|
||
// INFO: https://nodejs.org/dist/latest-v18.x/docs/api/http.html#serverrequesttimeout | ||
// Set the server request timeout to 0 to disable it. This was default behaviour pre Node.js 18.x | ||
server.requestTimeout = 0 |
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.
I think it makes sense to have the timeout disabled, provided the node is always behind a reverse proxy and not exposed on a public interface.
Alternatively we can have a non zero timeout, but larger value of say 20min, better than default of 5min?
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.
Or disabled timeout but make sure docs and our default deployment configs do not expose public interface.. ?
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.
My initial idea was to set a env
var as a parameter named MAX_REQUEST_TIMEOUT = 20 * 60 * 1000
(20 mins in milliseconds) so that it can be set by the user.
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.
LGTM, I have also tested the fix locally
addresses #5081
@types/node
dependency version to18.6.0
Context
There is a server.requestTimeout configuration option in the
http.Server
instance (rememberexpress
is just a wrapper around nodejs native HTTP Server), from the nodejs official documentation what this option means:The default value for the option is 300s for node versions
>=18.x
, while for the previous version the default value was 0 (no timeout)So, the uploads failing problem started to occur when we updated the Nodejs from
v14
tov18.6.0
in #4778This timeout value of 300s also validates Ignazio's investigation that Colossus aborts errors after ~5 minutes.