Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

'open' can be emitted after 'close' #23133

Closed
ronag opened this issue Sep 27, 2018 · 22 comments
Closed

'open' can be emitted after 'close' #23133

ronag opened this issue Sep 27, 2018 · 22 comments
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem.

Comments

@ronag
Copy link
Member

ronag commented Sep 27, 2018

'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 addaleax added the fs Issues and PRs related to the fs subsystem / file system. label Sep 28, 2018
@addaleax
Copy link
Member

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

@mscdex
Copy link
Contributor

mscdex commented Sep 28, 2018

Probably to emit 'open' before 'destroy'

@Fishrock123
Copy link
Contributor

cc @nodejs/streams

@Fishrock123 Fishrock123 added the stream Issues and PRs related to the stream subsystem. label Sep 28, 2018
@ronag
Copy link
Member Author

ronag commented Sep 28, 2018

I guess ‘ready’ is similar to ‘open’

@ronag
Copy link
Member Author

ronag commented Oct 2, 2018

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
Copy link
Member

mcollina commented Oct 3, 2018

@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
Copy link
Member Author

ronag commented Oct 3, 2018

@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
Copy link
Member Author

ronag commented Oct 8, 2018

@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
Copy link
Member Author

ronag commented Oct 8, 2018

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

@addaleax
Copy link
Member

addaleax commented Oct 8, 2018

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
Copy link
Member

mcollina commented Oct 8, 2018

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
Copy link
Member Author

ronag commented Oct 8, 2018

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
Copy link
Member

mcollina commented Oct 8, 2018

Can you make an example @addaleax?

@mcollina
Copy link
Member

mcollina commented Oct 8, 2018

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

@addaleax
Copy link
Member

addaleax commented Oct 9, 2018

@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
Copy link
Member

mcollina commented Oct 9, 2018

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

@addaleax
Copy link
Member

addaleax commented Oct 9, 2018

@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
Copy link
Member

mcollina commented Oct 9, 2018

Oh yes. We are in agreement then.

@ronag
Copy link
Member Author

ronag commented Oct 25, 2018

@mcollina can we label this as bug?

@mcollina mcollina added the confirmed-bug Issues with confirmed bugs. label Oct 26, 2018
@mcollina
Copy link
Member

@ronag definitely.

ronag added a commit to nxtedition/node that referenced this issue May 25, 2020
ronag added a commit that referenced this issue May 27, 2020
Refs: #23133

PR-URL: #29656
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
@ronag
Copy link
Member Author

ronag commented May 27, 2020

This was fixed in 54b36e4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. fs Issues and PRs related to the fs subsystem / file system. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants