s3, s3-multipart, tus: queue requests for soken token for remote files

[Murderlon]

Fixes #3640

Tested with 300+ files. But things can go wrong with a limit of 0 or 1 on a slow connection for instance. But it's still an improvement.

Before

After

[aduh95]

I'm getting this concerning error message in the console:

Socket.js:81 Error: Cannot mark a queued request as done: this indicates a bug
    at Object.done (RateLimitedQueue.js:91:15)
    at Function.<anonymous> (index.js:597:23)
    at emitAll (index.js:131:14)
    at Object.emit (index.js:33:7)
    at UppySocket.emit (Socket.js:68:19)
    at WebSocket.#handleMessage (Socket.js:78:12)

it does solve the websocket timeout issue

It does still timeout when I try to upload 300 files with a limit of 1 (it seems to work for the first 140 files then errors start popping up). I might still be an improvement over the current situation, but I wanted to point it out for completeness sake.

[Murderlon]

I tested this on Tus with Google Drive with a limit of 2 and I can successfully upload 313 files. Doing the same on the main branch will almost immediately result in errors and every retry will also have some failures. It might not be perfect for all combinations of settings but generally it should do the trick.

Perhaps we should discourage a limit of 1 (and 0)?

[aduh95]

I tested this on Tus with Google Drive with a limit of 2 and I can successfully upload 313 files.

I just tried and it's failing for me. In fact I think it's failing even with the default limit of 6, I guess my connection is not as fast as it needs to be.

Doing the same on the main branch will almost immediately result in errors and every retry will also have some failures. It might not be perfect for all combinations of settings but generally it should do the trick.

Same for me, I agree it's an improvement. Maybe we need to rename this PR as it looks like it does not fix timeout but rather the error we have on main.

Perhaps we should discourage a limit of 1 (and 0)?

Keep in mind that the RateLimiterQueue will set itself to 1 when getting a 429 response (then tries to increase its limit until it finds a sweet spot that does not trigger 429s anymore).
A limit of 0 is treated as Infinity, I think it's already discouraged (at least implicitly).

[mifi]

nice job!
I think if it's true that "RateLimiterQueue will set itself to 1 when getting a 429 response", then we need to find out why it fails with limit 1, because an uploader that becomes unstable only in some hard-to-test scenarios (429), is going to cause problems for us in the future and will be really tricky to fix later.

[mifi]

what's the purpose of calling this here? I thought this should only happen after await this.#queueRequestSocketToken(file) has returned?

[Murderlon]

If we retry then this is called again but we don't need a new server token, we already have it. In some other instances it's also possible to already have it

[mifi]

Ok, a comment could be nice

[mifi]

couldn't we just do return this.connectToServerSocket(file) ?

[aduh95]

Could the await make the file instance outdated? If the file was removed by the user while this code was waiting for the RateLimitedQueue to dispatch maybe? If so, it wouldn't be equivalent to return this.connectToServerSocket(file)

[Murderlon]

I think it's indeed safer to do it this way. It's also cheap so it doesn't matter much

[Murderlon]

I think if it's true that "RateLimiterQueue will set itself to 1 when getting a 429 response", then we need to find out why it fails with limit 1, because an uploader that becomes unstable only in some hard-to-test scenarios (429), is going to cause problems for us in the future and will be really tricky to fix later.

Note that is doesn't inherently fail on limit of 1! Only with lots of files on a slower connection it might happen. So this is still an improvement.