mafintosh / streamx

An iteration of the Node.js core streams with a series of improvements.
MIT License
226 stars 16 forks source link

`readable.readable` and `writable.writable` flags should be false after `.destroy()` is executed. #57

Closed sttk closed 2 years ago

sttk commented 2 years ago

API document is written as follows about readable.readable:

Is true if it is safe to call readable.read(), which means the stream has not been destroyed or emitted 'error' or 'end'.

and about writable.writable:

Is true if it is safe to call writable.write(), which means the stream has not been destroyed, errored or ended.

However, the following code outputs different result:

// test.js
const streamx = require('streamx');

const r = new streamx.Readable();
r.on('close', () => {
  console.log('r.destroyed', r.destroyed);
  console.log('r.readable', r.readable);
});
r.destroy();

const w = new streamx.Writable();
w.on('close', () => {
  console.log('w.destroyed', w.destroyed);
  console.log('w.writable', w.writable);
});
w.destroy();
$ node test.js
r.destroyed true
r.readable true
w.destroyed true
w.writable true

And this behavior is seemed to cause "premature close" error at this part in end-of-stream.

mafintosh commented 2 years ago

streamx streams are not node streams - the readable/writable props indicate if its a readable and writable stream. do you have a test case where this causes interop to break with big ecosystem things?

mafintosh commented 2 years ago

It's a feature that it hits that branch in eos btw. The stream is a readable stream that did NOT emit end, but closed - ie premature close (it errored without an explicit error).

martinheidegger commented 2 years ago

.write() in Node.js will throw an error after end. In streamx nothing will happen. In many pieces of Node.js code I saw blocks like if (!stream.writable) return close() (or similar) this practice is making an "upgrade" difficult to implement unless that whole logic block is turned into a streamx structure.

While I think it would make the life of users easier to implement .writable it should at least be noticed that this is a "documentation bug" as .writable and .readable are missing in the streamx documentation. It would be awesome to see documentation helping with an example how to best upgrade code currently relying on either property or the behavior of write/push after end.

sttk commented 2 years ago

@mafintosh Thank you for your above replies and also the replay to async-done's PR.

I thought that streamx works the same with node stream as far as it is written in Node.js API document.

Now I understand these bahaviors are as your design. And my problem in async-done might be able to solve by another way as you pointed out.

Thanks.