Skip to content

Add upload timeout #1377

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 2, 2019
Merged

Add upload timeout #1377

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
650b521
add upload timeout
eromano May 17, 2018
03e5efa
change after review
eromano May 18, 2018
e6630ec
clear timeout 100 percent
eromano May 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions lib/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,17 @@ Request.prototype.end = function(fn){
return this._end();
};

Request.prototype._setUploadTimeout = function () {
const self = this;

// upload timeout it's wokrs only if deadline timeout is off
if (this._uploadTimeout && !this._uploadTimeoutTimer) {
this._uploadTimeoutTimer = setTimeout(() => {
self._timeoutError('Upload timeout of ', self._uploadTimeout, 'ETIMEDOUT');
}, this._uploadTimeout);
}
};

Request.prototype._end = function() {
if (this._aborted) return this.callback(Error("The request has been aborted even before .end() was called"));

Expand Down Expand Up @@ -710,10 +721,12 @@ Request.prototype._end = function() {
// progress
const handleProgress = (direction, e) => {

clearTimeout(self._uploadTimeoutTimer);
if (e.total > 0) {
e.percent = e.loaded / e.total * 100;
} else if(e.total = 100){
Copy link
Contributor

Choose a reason for hiding this comment

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

You must write unit tests for it, because you've written non-working code second time in a row.

This is not a comparison, but an assignment (the condition is always true in this case).

Total is not a percentage, but amount of data to be transferred, so a comparison with a 100 byte file size doesn't make any sense.

If you're only clearing the timer without resetting, it doesn't add much value beyond response timeout. You could just add upload time to response timeout.

Copy link
Contributor Author

@eromano eromano May 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

yep you right, sorry I was in rush ;), honestly I guess we could add some linter in the project to avoid those mistakes and add it before the build. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I change it but is actually not clear to me the part :

If you're only clearing the timer without resetting, it doesn't add much value beyond response timeout. You could just add upload time to response timeout.

Do you have suggestions for the test?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you set timer once, and clear it once, then it's a one fixed length of time from start to finish. It doesn't change the timeout if there's one progress event or 100 progress events in the meantime.

The response timeout timer has the same inflexible behavior - it's set once, and cleared once. The only difference is that it's cleared in readystate rather than progress event, but that's not a major difference most of the time. So effectively your approach doesn't bring anything new.

The adaptive approach would clear timer and set a new one after every single progress event. (that's why I've suggested using a bound method, btw). That would make it adaptive to rate of events.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the "lint", we use unit tests as the lint. See existing tests. Write an endpoint for our built-in server, and then write code for success and failure case you expect.

Copy link
Contributor Author

@eromano eromano May 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

the lint would be different, ti will promote a common style across all the files http://jshint.com/docs/ . Tests are great but they are not style related they only guarantee that the functionality respect the wanted behaviour. If you see in your codebase there are different case where you use somewhere single/double quote ==/=== and other similars. Have a linter will guarantee a common style across all the contributors. Another usefull file for the indentation could be have an .editorconfig file where is described where any kind of file is formatted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kornelski your idea is to add an adaptive timeout similar the TCP one? http://www.pcvr.nl/tcpip/tcp_time.htm#21_0 if yes we should understand which strategy apply

clearTimeout(self._uploadTimeoutTimer);
}

e.direction = direction;
self.emit('progress', e);
};
Expand All @@ -731,12 +744,7 @@ Request.prototype._end = function() {
}

if(xhr.upload){
// upload timeout it's wokrs only if deadline timeout is off
if (this._uploadTimeout && !this._uploadTimeoutTimer) {
this._uploadTimeoutTimer = setTimeout(() => {
self._timeoutError('Upload timeout of ', self._uploadTimeout, 'ETIMEDOUT');
}, this._uploadTimeout);
}
this._setUploadTimeout();
}

// initiate request
Expand Down