dicer icon indicating copy to clipboard operation
dicer copied to clipboard

Dicer hangs when used with `stream/promises`' `pipeline`

Open indutny-signal opened this issue 3 years ago • 9 comments

Test case:

const { Readable } = require('stream');
const { pipeline } = require('stream/promises');
const Dicer = require('dicer');

async function main() {
  const r = new Readable({ read() {} });
  const d = new Dicer({ boundary: 'a' });

  d.on('part', async (part) => {
    part.resume();
  });

  r.push(`--a\r\nA: 1\r\nB: 1\r\n\r\n123\r\n--a\r\n\r\n456\r\n--a--\r\n`);
  setImmediate(() => {
    r.push(null);
  });

  const timer = setTimeout(() => {
    throw new Error('Should be canceled');
  }, 2000);

  await pipeline(r, d);

  clearTimeout(timer);
}

main();

indutny-signal avatar Mar 01 '22 08:03 indutny-signal

Investigated it and the issue appears to be the this.emit('finish') call when the part ends (end event). Apparently, the readable stream in this example never ends because the destination is unpiped before the 'end' event happens. In other words, we didn't quite finish writing yet, but Dicer happily reported that we did.

indutny-signal avatar Mar 01 '22 08:03 indutny-signal

This module is due for a rewrite now that node should have all of the necessary hooks to accomplish what the hacks were previously doing. I'm not sure when that will happen though.

mscdex avatar Mar 01 '22 15:03 mscdex

@mscdex I think I'm about to do that in TypeScript. Would you be interested in me contributing it back?

indutny-signal avatar Mar 01 '22 16:03 indutny-signal

More importantly, what ideas do you have for an API? Object mode streams instead of part event?

indutny-signal avatar Mar 01 '22 17:03 indutny-signal

What I had in mind was just removing the event hacks, using ES6 Classes, and other code cleanup (that doesn't cause noticeable performance regressions).... nothing breaking.

mscdex avatar Mar 01 '22 21:03 mscdex

Sounds good. I think I could try doing that, if you don't mind me submitting a PR.

indutny-signal avatar Mar 01 '22 22:03 indutny-signal

Have at it.

mscdex avatar Mar 01 '22 23:03 mscdex

@mscdex it turned out to be less complicated than I thought it would be: https://github.com/mscdex/dicer/pull/27

indutny-signal avatar Mar 02 '22 01:03 indutny-signal