Node: http: Reduce API surface

[ronag]

I would like to suggest that complete and aborted are unnecessary API surface and should be at least doc deprecated.

The user can keep of track of this state themselves by registering an 'aborted' handler.

In particular the exact semantics of e.g. complete is a slightly unclear and might cause more harm than use.

[ronag]

@nodejs/web-server-frameworks

[sam-github]

This is a common theme in event based APIs. Should the API report only changes in state, which is sufficient to deduce current state? Or should it also be possible to see the current state, without tracking it?

The pro of reducing API surface is less API surface :-). Nice.

The con is every user has to track the state, so if there are multiple observers of the object, that can be a burden. The other problem is when, for example, the object is passed to a utility function at some point. The utility function will not have had the opportunity to observe the object lifetime and track its state.

ChildProcess and cluster, (probably more examples) have a mixture of "current state" APIs as well as "change events". There is consistent demand for both styles.

All of which is to say, I'm -0 on this, without a bit more contextt/justification, it seems unnecessarily risky, and it can be useful to poll the current state.

Unless there is a technical reason tracking the state is difficult or error prone? But then again, that would be a good reason for node core to do it, and it _not_ to be left to users (we don't want to see an is-aborted equivalent of https://www.npmjs.com/package/on-finished).

[ronag]

The other problem is when, for example, the object is passed to a utility function at some point. The utility function will not have had the opportunity to observe the object lifetime and track its state.

Good points. In this specific case I would argue these properties just add confusion and there are more generic alternatives.

i.e.

aborted == stream.destroyed && !stream.readableEnded
complete == stream.readableEnded

The example in complete even looks wrong to me. If the response has not finished it should not be emitting 'end'. Which I guess further argues my point that these properties just lead to confusion.

(we don't want to see an is-aborted equivalent of npmjs.com/package/on-finished)

on-finished should not be required anymore given stream.writableFinished. I think the on-finished implementation is ambigious/broken and would personally discourage using it for Node 12+.

[jasnell]

I see two solid technical reasons to reduce the API surface:

1. These can fairly easily be moved to frameworks and
2. With HTTP/2 and the upcoming HTTP/3, both of which have very different implementations, we end up having to replicate the implementation of these bits to keep things consistent .. which has been error prone and difficult. For HTTP/3, for instance, I'm strongly consider simply not having an HTTP/3 specific or compatibility API in any way, and instead pushing that portion of things over to the frameworks.

[sam-github]

@ronag, you're the streams expert, but

aborted = (stream.destroyed && !stream.readableEnded)

surprised me. aborted is whether we aborted the http request, or whether the peer aborted the http request?

Anyhow, I'm not blocking, just pointing out some stuff to consider.

the on-finished implementation is ambigious/broken and would personally discourage using it for Node 12+.

Its used by lots of middleware that does HTTP request logging, and its more than a little painful for npm packages that support all LTS version of node to have to decide to use it or not based on matches against process.version...

So, this is a bit of an aside, but on-finished should be kept working (by changes in either Node.js or it) until its unnecessary on all supported (by us) Node.js versions, at which point we can tell packages to switch to a core API equivalent on all Node.js versions. <--- This is the general principle of how to update at a rate that doesn't break the ecosystem.

[ronag]

surprised me. aborted is whether we aborted the http request, or whether the peer aborted the http request?

'aborted' is basically ECONNRESET on IncomingMessage which will result in the object should be destroyed (destroyed) before emitting 'end' (readableEnded), i.e. stream.destroyed && !stream.readableEnded. Both of your examples goes under this.

Its used by lots of middleware that does HTTP request logging, and its more than a little painful for npm packages that support all LTS version of node to have to decide to use it or not based on matches against process.version...

on-finished should of course continue functioning and I don't see that we could ever get everyone stop using it. I'm just saying that I discourage it on newer node versions when possible.

[sam-github]

ECONNRESET on IncomingMessage which will result in the object should be destroyed (destroyed)

I thought destroyed meant .destroy() was called, but I guess that's only true for streams? Well, non-http streams?

I'm just saying that I discourage it on newer node versions when possible.

Unless there is simple code using documented APIs that works equivalently on 10.x and greater I'm going to keep enouraging it to be used, in the hopes that people write code that is robust over all supported Node.js versions. I guess if someone is writing an app (not a published package), and are absolutely sure their app will never run on 10.x, they can ignore 10s existence.

Is there equivalent code?

[ronag]

Is there equivalent code?

Yes.

function onFinished(res, cb) {
  if (res.writableFinished) cb()
  else res.on('finish', cb).on('error', cb)
}

The problem with on-finished is that it just checks for whether the response has been end():ed, i.e. writableEnded, not whether the data has actually been flushed, i.e. writableFinished.

Though this is getting a little off topic...

[ronag]

I thought destroyed meant .destroy() was called, but I guess that's only true for streams? Well, non-http streams?

No, that should be for http streams as well. destroy() should be called when the stream is "done". I noticed though that this is not always true at the moment for IncomingMessage. I would consider that a bug that should be resolved before deprecating these properties.

EDIT: Fix PR

[sam-github]

Maybe off topic... :-). Bringing it back:

aborted = (res.stream.destroyed && !res.stream.readableEnded);
complete = (res.stream.readableEnded);

1. ^---- works on 10.x and above?
2. ^---- @jasnell hopes that removing API surface will make http/2 and 3 easier... but if the replacement is the above code, will having to instead make these stream properties work the same on http/2 and http/3 be an improvement? In other words, maybe these offer an abstraction, so that even if the details of the underlying .stream (if its even there) are different, these properties have useable values. Or maybe they are legacy crud, and deserve to be deleted?

function onFinished(res, cb) {
  if (res.writableFinished) cb()
  else res.on('finish', cb).on('error', cb)
}

Maybe you meant this, but I want to be very, very explicit:

1. ^---- above code works on Node.js 10.x and above? It is guaranteed that if finish is emitted, that error will never be emitted afterwards?

I've reviewed code recently that just does res.on('finish', cb), lack of error checks was obvious, lack of check of res.writableFinished not so obvious.

I am generally in favour of changes to Node.js APIs, even backwards incompat ones, as long as they can be made gradually, and have a migration plan such that people can write code that works on all LTS node versions until the old behaviour drops off LTS, then move to the new APIs, and then we can delete the old ones.

I (like most) don't understand the streams internals well enough to know if this is the case for the properties you propose to deprecate here, but if it is, I'm good.

More feedback from web and http folks would be good, too.

[ronag]

^---- works on 10.x and above?

no

Or maybe they are legacy crud, and deserve to be deleted?

This. The problem with e.g. complete is that it is not well understood. Likewise with aborted vs destroyed which causes confusion and incorrect assumptions. It looks to me like even the docs are confused (or maybe I am).

--- above code works on Node.js 10.x and above? It is guaranteed that if finish is emitted, that error will never be emitted afterwards?

No, it only works in Node 12+.

I'm kind of at loss at what the argument here is? on-finished is not affected by these 2 properties.

EDIT: sorry, I did not notice on-finished uses complete. Though I still think it should be doc deprecateable and possibly runtime deprecated some time after 10 is not longer LTS.

[sam-github]

Doc deprecations are purely advisory (and mostly unnoticed), so no objections there, if there is a path towards runtime deprecation.

If there is a migration plan, I'm OK with runtime deprecation. A plan of "wait until 10.x is out of support when there will be a good alternative" is fine by me.

[ghermeto]

@ronag I'm confused why you suggest http-timer (which is used in got) to listen for aborted just a few days go.

https://github.com/nodejs/node/issues/32995#issuecomment-617919340

[ronag]

@ronag I'm confused why you suggest http-timer (which is used in got) to listen for aborted just a few days go.

@ghermeto Why does this confuse you?

[ghermeto]

s/confused/curious

So, what I really wanted to confirm was: do you want to reduce the API by removing the properties aborted and complete or do you also want to remove the event aborted?

But, reading the thread again, now it is clear that you meant only the properties and I agree.