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

Corking #18

Closed
romanzy313 opened this issue Jul 25, 2022 · 6 comments
Closed

Corking #18

romanzy313 opened this issue Jul 25, 2022 · 6 comments

Comments

@romanzy313
Copy link

You need to implement corking.
Basically wrap the ServerResponse in a callback inside cork.

    res.cork(() => {
      res.writeStatus(...)
      res.writeHeader(...)
      res.write(...);
    })

Its easy to do and will improve performance significantly. If you are having troubles, I can open a PR.

From the lib:
Corking a response is a performance improvement in both CPU and network, as you ready the IO system for writing multiple chunks at once. By default, you're corked in the immediately executing top portion of the route handler. In all other cases, such as when returning from await, or when being called back from an async database request or anything that isn't directly executing in the route handler, you'll want to cork before calling writeStatus, writeHeader or just write. Corking takes a callback in which you execute the writeHeader, writeStatus and such calls, in one atomic IO operation. This is important, not only for TCP but definitely for TLS where each write would otherwise result in one TLS block being sent off, each with one send syscall.

@endel
Copy link
Member

endel commented Jul 27, 2022

Hi @romanzy-1612, a PR would be very welcome for this!

@hunkydoryrepair
Copy link
Contributor

image
any movement on this? it's basically blocking us from upgrading to node 18 or colyseus 0.15

@endel
Copy link
Member

endel commented Jun 28, 2023

Hi @hunkydoryrepair, I've never managed to reproduce this, can you share how to reproduce it? 🙏

@Always-Pixels
Copy link
Contributor

Hi Endel, I just created a PR for this. Also, bumped the version of "uWebSockets.js" to v20 (from v19) to support Node 18.

@hunkydoryrepair
Copy link
Contributor

hunkydoryrepair commented Jun 28, 2023

Yeah, the issue appears when uWebSockets v20 is used. v19 doesn't work with Node 18 (just crashes), and our CICD platform is removing support for Node 16 in 2 months.

It actually works, but floods our logs with warning messages.

@endel
Copy link
Member

endel commented Jun 29, 2023

Thank you @Always-Pixels 🙏 #25 has been merged and published!

@endel endel closed this as completed Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants