gulpjs / async-done

Allows libraries to handle various caller provided asynchronous functions uniformly. Maps promises, observables, child processes and streams, and callbacks to callback style.
MIT License
70 stars 21 forks source link

chore: Add tests for streamx #58

Closed phated closed 2 years ago

phated commented 2 years ago

Closes #55

Blocked on a bug in one of these tests because streamx is causing a "premature close" error in end-of-stream.

cc @mafintosh

sttk commented 2 years ago

@phated This error might be due to streamx.

This error is caused at this position in end-of-stream.

And the following code (using streamx) outputs that stream._writableState.destroyed is undefined though the stream was destroyed.

const streamx = require('streamx');
const stream = streamx.pipeline(
  streamx.Readable.from('foo bar baz'),
  new streamx.Transform({
    transform: function(data, cb) {
      cb(new Error('fail'));
    }
  }),
  new streamx.Writable(),
);
stream.on('error', function(err)  {});
stream.on('close', function(err) {
  console.log('[close]', 'stream.destroyed', stream.destroyed);
  console.log('[close]', 'stream._writableState.destroyed', stream._writableState.destroyed);
});
----
% node test.js    
[close] stream.destroyed true
[close] stream._writableState.destroyed undefined

On the other hand, the following code (using pumpify) outputs that stream._writableState.destroyed is true.

const pumpify = require('pumpify');
const stream = pumpify(
  streamx.Readable.from('foo bar baz'),
  new streamx.Transform({
    transform: function(data, cb) {
      cb(new Error('fail'));
    }
  }),
  new streamx.Writable(),
);
stream.on('error', function(err)  {});
stream.on('close', function(err) {
  console.log('[close]', 'stream.destroyed', stream.destroyed);
  console.log('[close]', 'stream._writableState.destroyed', stream._writableState.destroyed);
});
----
% node test.js 
[close] stream.destroyed true
[close] stream._writableState.destroyed true

Otherwise, end-of-stream should probably look at stream.destroyed instead of stream._readableState.destroyed and stream._writableState.destroyed.

The cause is not stream._writableState.destroy property but stream._writableState.ended or stream.writable property.

mafintosh commented 2 years ago

I poked around your PR to see if this was a streamx issue, but it doesn't seem to be.

Internally you are calling the following in the failing test

eos(stream, { error: false }, callback)

That explicitly disables the error listener in streamx. It therefore doesn't get the "Fail" error, but streamx also doesn't end with the "everything was okay events" ie end/finish, so eos reports an premature close as the it only sees the close event.

This is by design and a good thing. If I missed something let me know.

sttk commented 2 years ago

@mafintosh Certainly, after removing eosConfig = { error: false } to eos, the test case which occured "premature close error" is passed.

@phated Can we remove this eosConfig? Even if it is done, all test cases are passed.

phated commented 2 years ago

@mafintosh thanks for looking into that! I don't exactly remember why I wanted/needed the { error: false } config, but all the tests seem to pass if I remove it (and I got to cleanup a workaround)