'open' can be emitted after 'close'

[ronag]

'open' can be emitted after 'close' for file streams which can cause some unexpected behaviours.

EDIT: Updated to after 'close' instead of after destroy().

[addaleax]

What’s the expected behaviour? To never emit open?

[mscdex]

Probably to emit 'open' before 'destroy'

[Fishrock123]

cc @nodejs/streams

[ronag]

I guess ‘ready’ is similar to ‘open’

[ronag]

Here is an example of where this easily causes a bug. I would like to assume that this is in some form a quite common pattern.

const rec = db.getRecord(blockId)
pipeline(
  req, 
  fs.createWriteStream(blockPath, { flags: 'wx' })
    .on('open', () => {
      // Oops, this can be called after rec.destroy() :/.
      rec.set({
        path: blockPath
      })
    }),
  err => {
    rec.destroy()
    callback(err)
  }
)

[mcollina]

@ronag is this limited to files? The current mechanism is not really checking anything like that as 'open' is specific to files, so we should really remove the listeners for 'open' inside _destroy() there.

What do you think? Would you like to assemble a PR?

[ronag]

@mcollina I just noticed there is something called ready which is similar to open but not limited to files. Not quite sure what the impact for that is.

I'll look into doing a PR for files & open.

[ronag]

@mcollina I still feel there are some things unclear here.

What exactly is the expected behaviour in regards to streams. The stream api seems to have a 'ready' event as well which is guess is analogous to fs 'open'. How are these kind of events expected to work?

i.e. if I call destroy on a stream before ready, should we emit ready/open? If not, isn't it weird that a close can be emitted without a ready/open? Is the expected behaviour defined somewhere?

What exactly is the expected state of a stream after calling destroy?

[ronag]

We also have connect/ready for Socket and ready for http2 which I am unsure how they behave at this moment.

[addaleax]

I think the semantics for ready should be the same everywhere.

One issue here is that this event always comes from the underlying implementation, not the streams layer itself. So, it may not be easy for the streams implementation to tell whether to respect that event or not?

[mcollina]

There is no ‘ready’ or ‘open’ event in Streams. Objects that inherits from
them should make sure these type of situations do not happen. However, the stream machinery cannot enforce if for backwards compatible manner.

In theory, after calling .destroy(), no new event should be emitted. There is a known problem regarding error events, but this is not the case.

The problem for fs is that we have scheduled opening a file, and when that happens the file is emitted. destroy happens in the meanwhile, because this is operation is not cancellable.

I think we
can make the fs stream not emit open at all if the stream is already destroyed.

[ronag]

In theory, after calling .destroy(), no new event should be emitted.

In theory, we wouldn't want close to be called either then? I think that would in practice break a lot of code (including pipeline)?

[mcollina]

Can you make an example @addaleax?

[mcollina]

@ronang that should be emitted by the .destroy cycle.

[addaleax]

@mcollina Sure – but example for what?

If you say streams shouldn’t emit these, should we maybe just guard the emit() calls with if (!this.destroyed)?

[mcollina]

The typical pattern for streams is inheritance, and I fear that protecting emit would likely cause some significant breakage.

[addaleax]

@mcollina I don’t understand… I’m not talking about changing emit() to be a noop function or so, I’m talking about the individual call sites in our own net, tls, fs, etc. code?

[mcollina]

Oh yes. We are in agreement then.

[ronag]

@mcollina can we label this as bug?

[mcollina]

@ronag definitely.

[ronag]

This was fixed in 54b36e4