Proper usage of Corking for best performance with uWebsockets.js

[kartikk221]

Hello,

I was looking to improve the performance of the hyper-express implementation on top of uWebsockets.js and realized that I am not properly corking the responses for best performance.

From what I have been able to gather from looking at issues, discussions and other resources in this respository, It seems that for responses which are sent synchronously before uWS.HttpRequest is returned, uWebsockets.js automatically corks the response. However, for when a .onAborted() callback is bound and the response is sent asynchronously, corking is not done automatically and should be done before sending the response asynchronously.

Based on the above information, I had the following questions:

• What is the cost to corking if any?
• Does corking have to be done for synchronous responses meaning when the response will be sent before the uWS.HttpRequest is returned in the initial route synchronous callback from uWS? If so when?
• Should corking always be done for asynchronous responses? If not, does whether we are going to be writing any custom status, headers or doing multiple .write() calls have a role in determining whether corking should be done?
• Should the size of the response body effect whether corking should be done? For example, should we cork() If we are just gonna call .send() once but with a decent size body and If so what is considered a size after which corking is important.

I apologize If any of the above questions may have been answered somewhere else in the resources for this repository but was hoping to get a very fine tuned use case for corking so it can be best implemented.

Thanks!

[e3dio]

From what I have been able to gather from looking at issues, discussions and other resources in this respository

Did you look at the User Manual? It explains everything you need to know in detail: https://github.com/uNetworking/uWebSockets/blob/master/misc/READMORE.md#corking

[kartikk221]

Maybe I'm missing something but that resource states sync responses are automatically corked and async responses should almost always be corked. While that is helpful, I wanted a bit more specifics as to scenarios in which cork can be used or omitted for the async cases or the sync cases hence the discussion post. But It seems it has been answered by @uNetworkingAB below and he will be adding warnings to the console output which should make optimizing much easier.

[uNetworkingAB]

• Zero (except for the fact JavaScript is a script so there is some small cost of calling the function)
• No, but it doesn't harm given above bullet point
• Yes
• No

[uNetworkingAB]

The warning is supposed to be as hard hitting of a slap, that is legally allowed without stepping into man slaughter realm. Disabling the warning makes the warning pointless

[e3dio]

Yesterday I tried corking a writestatus/header plus res.upgrade and it didn't work so better not spam me bunch of warnings if not possible to do Ok I am idiot it works fine #911 (comment)

Critical warning: Calling uWS.HttpResponse [.writeStatus, .writeHeader, .end, .write, .tryEnd, .upgrade, .endWithoutBody, or similar] outside of uWS.HttpResponse.cork callback is highly discouraged

[uNetworkingAB]

@e3dio report that as a bug then, it should work.

[kartikk221]

Yesterday I tried corking a writestatus/header plus res.upgrade and it didn't work so better not spam me bunch of warnings if not possible to do

Critical warning: Calling uWS.HttpResponse [.writeStatus, .writeHeader, .end, .write, .tryEnd, .upgrade, .endWithoutBody, or similar] outside of uWS.HttpResponse.cork callback is highly discouraged

I didn't even know you send a custom status code and headers with a request that gets upgraded to a websocket connection. Thought it always HTTP 101 switching protocols?

[e3dio]

writeStatus('101') before writeHeader() otherwise it will writeStatus('200') by default, you could set different status but then websocket will fail for client

[uNetworkingAB]

Alright I have a semi-good solution that allows shipping this in v20 right now.

Ideally I want a termination of the app on wrong use, but that would break semver minor. So what I am going to do is just add stderror prints warning of ill-use whenever they happen. This is backwards compatible yet makes it obvious when wrong usage is made

[kartikk221]

That would be great, Thanks!

[uNetworkingAB]

Waiting for CI, will release this so you can see for yourself when you get warnings

[uNetworkingAB]

Have made some fixes; write and tryEnd are not required to be corked, as there are cases where calling them outside of a cork is the best (such as when continuing a large write from onWritable, or anywhere else. Ideally the warning would be better at knowing exactly when it's ill-use but for now I just have to remove tryEnd, write from the checks.

Will make a new release when CI finishes

[uNetworkingAB]

No, I have done some thinking and .write and .tryEnd should also, always be corked. It might looks ridiculous in some cases such as this one:

res.cork(() => {
res.write("some single chunk");
});

but it is always better to be ridiculously corked, than not. The whole idea is telling the lib that "I'm going to write now". Sure, the naming could be changed from "cork" to "collect" and then we can add a special case "collectOne" for cases where there will only be 1 write. But it is always better to just be corked, as the library will internally pick whether to actually cork it or not. For instance large sends never go to cork buffer regardless of being corked or not, so in cases where you want to send big amounts of data, being corked will just be a bureaucratic detail.

That's why it is inherently impossible to expose uWS.js as Express interface or similar, since Express has no concept of corking, while it is a core concept of uWS - with this warning this concept only becomes more obvious and it always has been this central only that people don't give a shit about reading the user manual.

[kartikk221]

Sounds good!

[0x7d8]

how would you use this required corking in combination with .onWritable()? Since .cork takes a callback function I cant return to the writable what I got from .write() or .tryEnd(). Either Im tired or thats just not possible?

[uNetworkingAB]

No. All callbacks from uWS are already corked. It's in the documentation and the user manual. Nothing has changed here

[0x7d8]

I also thought that but im getting those new warnings when doing write in onWritable callback

[uNetworkingAB]

Then you have some await stuff inside onWritable, so your continuation is not really running inside onWritable. Any async continuation will run from within the microtask queue which is unwound from onWritable. We currently check if onWritable returns empty, but I'm pretty sure await will return a Promise, so it never sees it as empty, even if a Promise is entirely wrong to return (you are supposed to return a boolean). So fixing this check to be more strict, is a good idea.

There are a bunch of mis-use I see now, and we haven't caught it since we never had strict tests, assuming people would read the manual but people don't. So having strict checks that enforce the rules will be more important than writing text

[webcarrot]

There are a bunch of mis-use I see now, and we haven't caught it since we never had strict tests, assuming people would read the manual but people don't. So having strict checks that enforce the rules will be more important than writing text

@uNetworkingAB Many of us just copy pasted outdated examples like:

examples/VideoStreamer.js

😉

[jacsamg]

This discussion led me to read the manual. I must say, thank you.