mscdex / dicer

A very fast streaming multipart parser for node.js
MIT License
186 stars 36 forks source link

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

Open indutny-signal opened 2 years ago

indutny-signal commented 2 years ago

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 commented 2 years ago

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.

mscdex commented 2 years ago

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.

indutny-signal commented 2 years ago

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

indutny-signal commented 2 years ago

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

mscdex commented 2 years ago

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.

indutny-signal commented 2 years ago

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

mscdex commented 2 years ago

Have at it.

indutny-signal commented 2 years ago

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