onData and onWritable overloads that get the response first and the abort

[nigrosimone]

As discussed in uNetworking/uWebSockets.js#1349, only as an addition: nothing changes for existing code.

New overloads onData(res, chunk, maxRemainingBodyLength) and onWritable(res, offset) get the response first and the abort as a null response, until the response ends. So you need no onAborted and no shared flag like the shared_ptr<bool> in Crc32.cpp. They have their own slot: each one replaces the old handler of the same kind and the other way around, and the old ones work and cost as before (marked deprecated in the comments). onData(nullptr) and onWritable(nullptr) have their own overload, so they are not ambiguous.

The tests in tests/HttpResponse.cpp check the abort (waiting GET, after the body, all handlers in order, none after end), the next request on the same socket, replacing old with new and new with old, nullptr, and streaming with the new onWritable.

[uNetworkingAB]

Interesting but we can also just pass the HttpResponse into onData and onWritable so you don't need to capture res, and then a nullptr for res can mean abort (there is no socket anymore)

[uNetworkingAB]

And if the HttpResponse is first argument it will immediately break all v20 apps so that they immediately see that v21 is not compatible with v20 and will need some work, instead of working fine until you get a null that they haven't handled in their business code.

[nigrosimone]

done eec99af

[nigrosimone]

One cost with onData as the only way to wait. Today a handler that only waits (a db call, a timer) attaches onAborted, which costs nothing until an abort. With v21 it attaches onData, and a request without body still gets the empty last chunk (the else in HttpParser that emits {}, 0). So every async GET pays one more call of the handler, and in uWS.js one more call into JS with an ArrayBuffer: a MakeCallback is about 120 ns with Node today.

Proposal: a request with neither Content-Length nor Transfer-Encoding gets no data call, only the abort. Content-Length: 0 still gets its empty last chunk. The price is that a generic handler reading the body (onData on an any route) gets no "end of body" on a GET, so it has to check the method or the headers.

Do you want it in this PR?

[uNetworkingAB]

So every async GET pays one more call of the handler [...] MakeCallback

[nigrosimone]

I laughed more than necessary at this. I take it as a yes, so I am adding it to this PR.

One change from what I wrote: only a GET or HEAD without Content-Length and Transfer-Encoding skips the empty last chunk. A POST without Content-Length still gets it, so collectBody and the handlers that wait for the last chunk do not hang, and a GET with a body works as before.

[uNetworkingAB]

I changed my mind, I don't want v21. I want to make this change strictly an addition, to leave it backwards compatible for those who don't use the 2 new onData, onWritable variants.

It's possible to make backwards compatible; similarly to how onDataV2 is the base for onData, we just need to make a new onDataWithSocket (or whatever name!) that gives the (res, chunk, maxRemainingBytes) and acts as the base for both onDaraV2 and onData.

So the big issue now is: find good function names.

I rather have a long and shitty name that is an ADDITIVE than a good name that breaks backwards compatibility (at this point a v21 would be suicide - it would cut many years of backwards compatibility

[uNetworkingAB]

Well the obvious solution is to just use an overload that takes a callback of a different signature, and mark the old variants as deprecated. That solves the issue perfectly in C++ land, but not sure how that works in JS. In JS, an overload is dynamic and needs dynamic runtime checks to figure out what version you're using, right? Even if you type it with TypeScript, the actual runtime still goes through JS.

[uNetworkingAB]

In JS you would need to check the arity of the callback being passed in, that's a pretty lean check I guess? Basically arity of 3 is the new overload and 2 is the old.

[nigrosimone]

OK, no v21. I reworked both PRs as an addition, nothing changes for existing code.

I tried the arity in JS first: reading length of the callback from C++ costs ~70 ns per onData/onWritable call, more than the call itself (~55 ns), and everybody pays it, also who never uses the new ones. So I went with names, the same in C++ and JS: onDataOrAborted and onWritableOrAborted, your names from #1349.

In C++ the old ones are marked deprecated in the comments. In JS not yet: onAborted is still needed for collectBody and HTTP/3.

[uNetworkingAB]

I tried the arity in JS first: reading length of the callback from C++ costs ~70 ns per onData/onWritable call, more than the call itself (~55 ns)

Well that sucks but in C++ you can easily make it work with overload and then you get nice original names onData, onWritable and you keep backwards compatibility, that's what I'll do first, and then JS can be an afterthought. For instance, one could add onData overload to JS but also add onDataOrAborted as a JS-only non-overloaded version.

Basically; JS should not dictate how the C++ lib is written (then we would end up with nonsense code that goes against all rational thinking). Naming things onDataV2, onDataV3 is just ridiculous when C++ has zero cost overloads. If anything, those names could land in JS (only).

You could even do something like uWS.disableDeprecatedOverloads() to disable the old overloads. Again, JS should not dictate how the C++ lib is built.

[uNetworkingAB]

The fact res comes as argument is objectively better. It requires less capture space (less likely to spill over as malloc), and passing nullptr or nullopt makes an excellent indication for "aborted". And since aborted comes from within here, anything that async gathers data do not need to capture the shared_ptr aborted.

Only thing is, the name should be onData and be an overload here in C++. I am still thinking about the exact details (for instance do we want to pass std::optional to fully mark that there can come nullopt. That's probably the modern safer way.

[uNetworkingAB]

Also, if the "base" function onData that takes CB of arity 3 is called with nullopt on abort, we don't even need to overwrite the onAborted callback, we can just change the behavior so that the lib calls into onData on aborts (as well as onAborted). That makes is fully backwards compatible, if we then implement the old onData so that it skips calls that have nullopt.

The only thing to think about now is the onData("", 0) that is emitted on GET having no body. That thing needs some thinking because it still needs to be backwards compatible. So it probably still need that behavior.

Hmm..

[nigrosimone]

Done: in C++ they are now overloads of onData and onWritable (plus one for nullptr, so onData(nullptr) is not ambiguous). onDataOrAborted and onWritableOrAborted stay only in JS (#1351).

About compatibility, it already works like you describe: the new handler has its own slot, so the old onData, onDataV2, onWritable and onAborted are unchanged and cost the same, only the new one gets the abort. The empty chunk on a GET without body still comes to both.

About std::optional, as you prefer: now it passes a null pointer.