Skip to content
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

test/client-request.js, 'request dump' test case always takes ~ 5 seconds #3120

Closed
Uzlopak opened this issue Apr 15, 2024 · 5 comments · Fixed by #3135
Closed

test/client-request.js, 'request dump' test case always takes ~ 5 seconds #3120

Uzlopak opened this issue Apr 15, 2024 · 5 comments · Fixed by #3135
Labels
bug Something isn't working

Comments

@Uzlopak
Copy link
Contributor

Uzlopak commented Apr 15, 2024

Is there a reason, that 'request dump' takes 5 seconds but 'request dump big' just 30ms. Also I doubt that 'request dump big' is correctlly written, as it just sends data till reaching the highwatermark.

So the 5 seconds are kind of fishy. Maybe it keeps the connection open for some reason open? Maybe Maybe an indicator for a bug?

@Uzlopak Uzlopak added the bug Something isn't working label Apr 15, 2024
@Uzlopak
Copy link
Contributor Author

Uzlopak commented Apr 15, 2024

The same with 'request DELETE, content-length=0, with body' in test/content-length.js

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Apr 17, 2024

I investigated it further. The tests will run faster if I manually set shouldKeepAlive to false.

I guess this is also related to issue-803, as it never sending the whole payload to the client.

Also i have the fealing that contentLenght is not stored properly as an attribute . It is set as empty string. The numbers are added.

@mweberxyz
Copy link
Contributor

mweberxyz commented Apr 18, 2024

Dunno if relevant, but if you call dump({ limit: 4 }) (or any number less than the Content-Length) in the test it'll also finish right away.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Apr 18, 2024

If you set the dump limit to 4, then it will result in an error, which gets passed to noop. Thats why it is finishing early.

@mweberxyz
Copy link
Contributor

mweberxyz commented Apr 18, 2024

Seems working as expected then if I am understanding everything correctly. If dump is able to consume the entire response, then the socket is clear for another request, and the client's disconnect event won't fire until the keep alive timeout.

For 'request dump', would it be preferable to replace

    client.on('disconnect', () => {
      t.strictEqual(dumped, true)
    })

with

    client.on('drain', () => {
      setImmediate(() => t.strictEqual(dumped, true))
    })

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants