Repository navigation
http: Reduce API surface #33118
Description
Activity
@nodejs/web-server-frameworks
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-abortedequivalent of https://www.npmjs.com/package/on-finished).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.readableEndedThe example in
completeeven 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+.I see two solid technical reasons to reduce the API surface:
- These can fairly easily be moved to frameworks and
- 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.
Reacted by Tomas Della Vedova@ronag, you're the streams expert, but
aborted = (stream.destroyed && !stream.readableEnded)surprised me.
abortedis 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.
surprised me. aborted is whether we aborted the http request, or whether the peer aborted the http request?
'aborted'is basicallyECONNRESETonIncomingMessagewhich 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-finishedshould 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.ECONNRESET on IncomingMessage which will result in the object should be destroyed (destroyed)
I thought
destroyedmeant.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?
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-finishedis that it just checks for whether the response has beenend():ed, i.e.writableEnded, not whether the data has actually been flushed, i.e.writableFinished.Though this is getting a little off topic...
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 forIncomingMessage. I would consider that a bug that should be resolved before deprecating these properties.EDIT: Fix PR
Maybe off topic... :-). Bringing it back:
aborted = (res.stream.destroyed && !res.stream.readableEnded); complete = (res.stream.readableEnded);- ^---- works on 10.x and above?
- ^---- @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:
- ^---- above code works on Node.js 10.x and above? It is guaranteed that if
finishis emitted, thaterrorwill 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 ofres.writableFinishednot 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.
^---- works on 10.x and above?
no
Or maybe they are legacy crud, and deserve to be deleted?
This. The problem with e.g.
completeis that it is not well understood. Likewise withabortedvsdestroyedwhich 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-finishedis not affected by these 2 properties.EDIT: sorry, I did not notice
on-finishedusescomplete. Though I still think it should be doc deprecateable and possibly runtime deprecated some time after 10 is not longer LTS.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.
Reacted by Robert Nagy@ronag I'm confused why you suggest
http-timer(which is used ingot) to listen forabortedjust a few days go.- Reacted by Robert Nagy
This issue has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.This issue has been automatically closed after 30 days of inactivity following its stale status (no activity for a total of 120 days).
If this is still relevant, feel free to reopen it or leave a comment with additional details so we can continue the discussion.
I would like to suggest that
completeandabortedare 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.
completeis a slightly unclear and might cause more harm than use.