Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Streams2 Readable API confusion #4935

Closed
kanongil opened this issue Mar 6, 2013 · 3 comments
Closed

Streams2 Readable API confusion #4935

kanongil opened this issue Mar 6, 2013 · 3 comments

Comments

@kanongil
Copy link

kanongil commented Mar 6, 2013

In general I like the new Readable API, but I think there is an issue with the use of the 'readable' event.

Currently consumers are encouraged to read() data synchronously when the event is signaled, however this means that any other listeners will have a different state depending on whether they are called before or after the one with the read(). Ie. if a listener is scheduled after the one doing read(), it will likely no longer be in a readable state.

I'm not sure what the correct solution is. A quick fix is to limit the 'readable' event to only one listener, but I'm not sure this is appropriate.

Another related issue, is how 'readable' interacts with pipe() and unpipe(), which is different depending on the order. Ie.

r.pipe(w);
r.on('readable', fn);

is not the same as

r.on('readable', fn);
r.pipe(w);

None of these work, as I would expect. I would expect that 'readable' is only called once there are no more pipes on the stream, and it hasn't yet ended, and has more data.

@indutny
Copy link
Member

indutny commented Mar 6, 2013

Are you, in general, trying to read from stream simultaneously with another reader?

@kanongil
Copy link
Author

kanongil commented Mar 6, 2013

@indutny No, I'm not.

@isaacs
Copy link

isaacs commented Mar 7, 2013

The 'readable' event is a part of how pipe() works, so what you're asking for isn't really possible.

If you read() from a stream that is flowing through a pipe chain, then you'll probably mess stuff up. This is the same as any other unix stream in that way. You CAN pipe to multiple writers, so if you really need to be able to consume the bytes without screwing up the flow, you can do var myCopy = new stream.PassThrough(); src.pipe(myCopy); myCopy.on('readable', etc)

@isaacs isaacs closed this as completed Mar 7, 2013
richardlau pushed a commit to ibmruntimes/node that referenced this issue Feb 29, 2016
* buffer:
  - You can now supply an encoding argument when filling a
    Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying
    an existing Buffer will also work with
    Buffer#fill(buffer[, start[, end]]). See the API documentation for
    details on how this works. (Trevor Norris) nodejs#4935
  - Buffer#indexOf() no longer requires a byteOffset argument if you
    also wish to specify an encoding:
    Buffer#indexOf(val[, byteOffset][, encoding]).
    (Trevor Norris) nodejs#4803
* child_process: spawn() and spawnSync() now support a 'shell' option
  to allow for optional execution of the given command inside a shell.
  If set to true, cmd.exe will be used on Windows and /bin/sh
  elsewhere. A path to a custom shell can also be passed to override
  these defaults. On Windows, this option allows .bat. and .cmd files
  to be executed with spawn() and spawnSync(). (Colin Ihrig) nodejs#4598
* http_parser: Update to http-parser 2.6.2 to fix an unintentionally
  strict limitation of allowable header characters.
  (James M Snell) nodejs#5237
* dgram: socket.send() now supports accepts an array of Buffers or
  Strings as the first argument. See the API docs for details on how
  this works. (Matteo Collina) nodejs#4374
* http: Fix a bug where handling headers will mistakenly trigger an
  'upgrade' event where the server is just advertising its protocols.
  This bug can prevent HTTP clients from communicating with HTTP/2
  enabled servers. (Fedor Indutny) nodejs#4337
* net: Added a listening Boolean property to net and http servers to
  indicate whether the server is listening for connections.
  (José Moreira) nodejs#4743
* node: The C++ node::MakeCallback() API is now reentrant and calling
  it from inside another MakeCallback() call no longer causes the
  nextTick queue or Promises microtask queue to be processed out of
  order. (Trevor Norris) nodejs#4507
* tls: Add a new tlsSocket.getProtocol() method to get the negotiated
  TLS protocol version of the current connection. (Brian White) nodejs#4995
* vm: Introduce new 'produceCachedData' and 'cachedData' options to
  new vm.Script() to interact with V8's code cache. When a new
  vm.Script object is created with the 'produceCachedData' set to true
  a Buffer with V8's code cache data will be produced and stored in
  cachedData property of the returned object. This data in turn may be
  supplied back to another vm.Script() object with a 'cachedData'
  option if the supplied source is the same. Successfully executing a
  script from cached data can speed up instantiation time. See the API
  docs for details. (Fedor Indutny) nodejs#4777
* performance: Improvements in:
  - process.nextTick() (Ruben Bridgewater) nodejs#5092
  - path module (Brian White) nodejs#5123
  - querystring module (Brian White) nodejs#5012
  - streams module when processing small chunks (Matteo Collina) nodejs#4354

PR-URL: nodejs/node#5295
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants