file-box icon indicating copy to clipboard operation
file-box copied to clipboard

fix: 🐛 fix stuck in http request

Open binsee opened this issue 2 years ago • 10 comments

Fixes: #80

binsee avatar Feb 02 '23 08:02 binsee

Also I have done a little test

    stream
            .on('abort', console.log)
            .on('connect', console.log)
            .on('continue', console.log)
            .on('information', console.log)
            .on('response', console.log)
            .on('socket', console.log)
            .on('timeout', console.log)
            .on('upgrade', console.log)
            .on('close', console.log)
            .on('drain', console.log)
            .on('error', console.log)
            .on('finish', console.log)
            .on('pipe', console.log)
            .on('unpipe', console.log)

none of this were fired when network disconnected

hcfw007 avatar Feb 02 '23 09:02 hcfw007

See #80 for updated results

binsee avatar Feb 02 '23 17:02 binsee

@huan I updated the test. And made some modifications with reference to the nodejs code, it should work fine now. However, I still think 60 seconds is too long and 5~10 seconds is perfectly sufficient. Too long a timeout and it will just wait in vain and throw an error. This can be learned from should not timeout and should timeout in the test.

image image

binsee avatar Feb 08 '23 11:02 binsee

https://betterstack.com/community/guides/scaling-nodejs/nodejs-timeouts/

In browsers, fetch() usually times out after a set period of time which varies amongst browsers. For example, in Firefox this timeout is set to 90 seconds by default, but in Chromium, it is 300 seconds.

huan avatar Feb 10 '23 06:02 huan

Ok, I don't think there is enough testing in various scenarios now, and there is no need to set the default value too small. In use, I can set the environment variable FILEBOX_HTTP_TIMEOUT as needed to achieve the effect I want, which is enough. So are there any other questions now? Hope this can be merged sooner, to solve the problem I'm having.

binsee avatar Feb 12 '23 14:02 binsee

The latest problem is that the unit test clock is not mocked successfully: It will take a long time to finish the test, instead of seconds.

We have to figure out how to make the clock mock work before it can be merged.

huan avatar Feb 13 '23 06:02 huan

I've spent a day researching this problem, but didn't find a solution. Therefore, I changed the test to run concurrently to reduce the overall time taken by this test. Looks like looking at the source code of the nodejs project is required to find a clue, but I don't have enough time to do so right now. Can we merge this first and then optimize the test time?

binsee avatar Feb 13 '23 07:02 binsee

I investigated the source code of nodejs and found that the problem should be that socket.setTimeout uses the built-in setUnrefTimeout instead of the usual setTimeout. Regardless of sion or jest, they only hook the common setTimeout, and there is no hook built-in setUnrefTimeout, so it cannot be mocked.

  • This is the socket's setTimeout call chain:

https://github.com/nodejs/node/blob/v16.x/lib/_http_client.js#L926-L952

https://github.com/nodejs/node/blob/v16.x/lib/net.js#L533

https://github.com/nodejs/node/blob/v16.x/lib/net.js#L74-L85

https://github.com/nodejs/node/blob/v16.x/lib/net.js#L237-L265

https://github.com/nodejs/node/blob/v16.x/lib/internal/timers.js#L372-L380

  • This is the general setTimeout:

https://github.com/nodejs/node/blob/v16.x/lib/timers.js#L140-L168

binsee avatar Feb 13 '23 07:02 binsee

You do not have to keep convincing me that this is impossible, as I believe everything should be testable.

Let me try to figure it out myself later.

Thanks.

huan avatar Feb 13 '23 08:02 huan

Have you found any good solution?

binsee avatar Feb 16 '23 06:02 binsee