Node: stream: enforce event rules

[ronag]

Proposal.

Override emit for Stream in order to enforce the following rules:

• only 'error' or 'close' after destroyed.
• only 'close' after 'error'

Could look something like:

Stream.prototype.emit = function(name, ...args) {
  const r = this._readableState;
  const w = this._writableState;
  const closeEmitted = (r && r. closeEmitted) || (w && w. closeEmitted); // This doesn't exist. 
  if (closeEmitted) {
    return;
  }
  const errorEmitted = (r && r. errorEmitted) || (w && w. errorEmitted);
  if (errorEmitted && name !== 'close') {
    return;
  }
  const destroyed = (r && r.destroyed) || (w && w.destroyed);
  if (destroyed && name !== 'close' && name !== 'error') {
    return;
  }
  EE.emit.call(this, name, ...args);
}

This would solve some subtle bugs such as e.g. fs streams emitting open after destroy() and alike.

The risk is that we are not fully aware how this can break "incorrect" streams and their usage in the wild.

[sam-github]

That makes me really nervous. Could all uses of emit in our code bases be changed to a Stream#checkedEmit() that behaves as you suggest?

"enforce" means: discards invalid emits, or does it mean throws error in invalid emits?

[ronag]

Possibly has some performance impact. Is it possible to implement a fast path for e.g. 'data'.

[ronag]

That makes me really nervous. Could all uses of emit in our code bases be changed to a Stream#checkedEmit() that behaves as you suggest?

I like that idea.

"enforce" means: discards invalid emits, or does it mean throws error in invalid emits?

No, just swallow.

[sam-github]

I'm not stream expert, but forcing state management through the whole code-base to see if its OK to emit something sounds not as useful as an idempotent emits. Changing the emit seen by users might cause confusion. But then again, if it enforces a contract that, when broken, causes other subtle bugs or outright chaos, maybe its not so bad an idea.

@nodejs/streams

[ronag]

causes other subtle bugs or outright chaos, maybe its not so bad an idea.

From my own experience. I've told my colleagues to not simply assume the "contract" and either check the node code or write their code in a way that is agnostic. I've had a few crashes in production with the fs streams before I learned the lesson myself :).

Another advantage is that it makes streams easier to implement and easier to reason about without lots of code checking before various emits.

But yes, there is a bit of a hidden magical feel to it.

[mcollina]

Unfortunately I fear that this is going to cause a slowdown in certain hot paths, e.g. 'data' or 'readable'. Moreover, emit is a leaky abstraction: a user could try to emit('error', err) for whatever reason and I do not think we have any rights to swallow that (note that this is different from adding a 'error' handler in finished/pipeline, because that event could still be listened to).

Overall, I'm -1.

[ronag]

@mcollina: Are you -1 on adding a checkedEmit as well? We could not use it in hot paths and then it's up to the stream implementor to enforce the contract.

[mcollina]

@ronag I would be more inclined to have that _throw_ instead of swallowing. Essentially if those happens... they are likely bugs.

[mcollina]

Adding checkedEmit might help in fixing some "old" issues.

[ronag]

@ronag I would be more inclined to have that throw instead of swallowing. Essentially if those happens... they are likely bugs.

I would prefer that as well... but going in that direction will actually break existing code that currently works.

[ronag]

The middleground is to swallow and emit a warning?

[mcollina]

I would like to see how many tests we break if we throw in core instead.
However I would not invest time in this right now, as it's going to be hard to get this landed.

[ronag]

@mcollina: For context. I was hoping to use this as a simpler fix to the fs streams emitting open after destroy(). But we can go with an explicit fix as well.

[mcollina]

I would prefer an explicit fix

Il giorno gio 15 ago 2019 alle 11:52 Robert Nagy [e-mail]
ha scritto:

@mcollina https://github.com/mcollina: For context. I was hoping to use
this as a simpler fix to the fs streams emitting open after destroy().
But we can go with an explicit fix as well.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/nodejs/node/issues/29125?email_source=notifications&email_token=AAAMXYZ7LS6YEABBYSHSV2DQEURNFA5CNFSM4ILWWYC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4LMHIY#issuecomment-521585571,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAMXY5WSTXKPVT63AJVHTTQEURNFANCNFSM4ILWWYCQ
.

[ronag]

I'm closing this for now. We might look into it at some point in the future.