Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
indutny-signal opened this issue Mar 1, 2022 · 8 comments · May be fixed by #27
Open

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

indutny-signal opened this issue Mar 1, 2022 · 8 comments · May be fixed by #27

Comments

@indutny-signal
Copy link

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
Copy link
Author

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
Copy link
Owner

mscdex commented Mar 1, 2022

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
Copy link
Author

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

@indutny-signal
Copy link
Author

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

@mscdex
Copy link
Owner

mscdex commented Mar 1, 2022

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
Copy link
Author

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

@mscdex
Copy link
Owner

mscdex commented Mar 1, 2022

Have at it.

indutny-signal pushed a commit to indutny-signal/dicer that referenced this issue Mar 2, 2022
Instead overriding `emit` method and using `this._realFinish` to detect
unexpected end of data - implement `_final` method and track the logic
there.

The only behavior change from the use of `_final` is that we would no
longer emit `finish` when the input data is terminated early. Instead we
would emit `error` as before and stop the processing. Note that this is
the behavior provided by `stream.Writable`, and it is thus conformant
with the specification.

Additionally, replace uses of private `_events` property with either
straight `emit()` with return value check, or `listenerCount()` in the
situations where there might be an overhead from the constructed event
arguments.

Fix: mscdex#26
@indutny-signal indutny-signal linked a pull request Mar 2, 2022 that will close this issue
@indutny-signal
Copy link
Author

@mscdex it turned out to be less complicated than I thought it would be: #27

indutny-signal pushed a commit to indutny-signal/dicer that referenced this issue Mar 2, 2022
Instead overriding `emit` method and using `this._realFinish` to detect
unexpected end of data - implement `_final` method and track the logic
there.

The only behavior change from the use of `_final` is that we would no
longer emit `finish` when the input data is terminated early. Instead we
would emit `error` as before and stop the processing. Note that this is
the behavior provided by `stream.Writable`, and it is thus conformant
with the specification.

Additionally, replace uses of private `_events` property with either
straight `emit()` with return value check, or `listenerCount()` in the
situations where there might be an overhead from the constructed event
arguments.

Fix: mscdex#26
indutny-signal pushed a commit to indutny-signal/dicer that referenced this issue Mar 2, 2022
Instead overriding `emit` method and using `this._realFinish` to detect
unexpected end of data - implement `_final` method and track the logic
there.

The only behavior change from the use of `_final` is that we would no
longer emit `finish` when the input data is terminated early. Instead we
would emit `error` as before and stop the processing. Note that this is
the behavior provided by `stream.Writable`, and it is thus conformant
with the specification.

Additionally, replace uses of private `_events` property with either
straight `emit()` with return value check, or `listenerCount()` in the
situations where there might be an overhead from the constructed event
arguments.

Replace `emit('error', error)` with `destroy(error)` calls as well, as
otherwise the behavior is undefined.

Fix: mscdex#26
indutny-signal pushed a commit to indutny-signal/dicer that referenced this issue Jun 25, 2024
Instead overriding `emit` method and using `this._realFinish` to detect
unexpected end of data - implement `_final` method and track the logic
there.

The only behavior change from the use of `_final` is that we would no
longer emit `finish` when the input data is terminated early. Instead we
would emit `error` as before and stop the processing. Note that this is
the behavior provided by `stream.Writable`, and it is thus conformant
with the specification.

Additionally, replace uses of private `_events` property with either
straight `emit()` with return value check, or `listenerCount()` in the
situations where there might be an overhead from the constructed event
arguments.

Replace `emit('error', error)` with `destroy(error)` calls as well, as
otherwise the behavior is undefined.

Fix: mscdex#26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants