Add upload timeout

[eromano]

Upload timer

#1375

New PR made from the discussion about to add a new upload timer that could be triggered only in case of long Upload chunk delay if the deadline timer is off

[kornelski]

You're not resetting the timer, so it allows infinitely long upload time after first event (so if e.g. 1% is uploaded quickly, nothing will stop remaining 99% taking forever)

[eromano]

Maybe I misunderstood should I clear only if is 100%?

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

[kornelski]

I suggest assigning that callback to a property and using the property when resetting the timer to avoid recreating the closure frequently.

[eromano]

ok 👍

[kornelski]

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.

[eromano]

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?

[eromano]

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?

[kornelski]

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.

[kornelski]

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.

[eromano]

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

[eromano]

@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

[kornelski]

It'd still be great to have tests for it, otherwise it's easy to break it accidentally.

[Rychu-Pawel]

This feature is undocumented in the docs but before we add this to the docs could you please answer my questions:

• Do I see correctly, in the code, that it doesn't work in the Node environment? I believe we are missing _setUploadTimeout() somewhere in the node part.

• I believe this comment in the code is not valid:

• • upload is the time since last bit of data was sent or received. This timeout works only if deadline timeout is off

Could you please confirm that I see correctly that this feature sets the timeout for the upload but the timer is never "extended" in the handleProgress event? It is only cleared when the upload is completed. Actually... I believe there is also an e.direction check missing in that event handler? If not then I believe it shouldn't be called upload timeout. More a transfer timeout?

• I believe this comment is not valid either and is misleading:

// upload timeout it's wokrs only if deadline timeout is off

It (should?) work(s) fine when the deadline is set. It is simply designed to be something else.

I really need support for this feature in my Node project so I would like to be sure this functionality works as expected.