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

Fix write-after-close and other bugs #61

Merged
merged 6 commits into from
Feb 24, 2021
Merged

Fix write-after-close and other bugs #61

merged 6 commits into from
Feb 24, 2021

Conversation

rhansen
Copy link
Collaborator

@rhansen rhansen commented Feb 24, 2021

Multiple commits (please do not squash) to fix a few bugs and improve code health.

Bugs fixed:

  * Close the streams via `destroy()` rather than manually closing the
    underlying file descriptors. This avoids a Node.js SIGABRT crash
    caused by an EBADF error from writing to a closed file descriptor.
  * Track the number of in-flight writes so that the `drain` event is
    not emitted unless the writes are actually drained.
  * Wait until drained before closing the streams.
  * Rather than building a string with an O(N^2) algorithm and an
    arbitrary 1000 character limit, use `writable.cork()` and
    `writable.uncork()` to let Node.js efficiently batch writes.
  * Use a Map to efficiently implement the write queue. Maps iterate
    in insertion order so there is no behavior change.
Maps iterate in insertion order so there is no behavior change.
@felixge
Copy link
Owner

felixge commented Feb 24, 2021

@rhansen 👋 I'm not actively maintaining this repo anymore, but your changes seem nice, so I added you as a contributor. I'll leave it to you to merge this PR as-is or wait to see if you can get a review from somebody.

If you want I'm also happy to give you access to the npm package so you can publish new releases.

@rhansen rhansen merged commit ee3952a into felixge:master Feb 24, 2021
@rhansen rhansen deleted the fixes branch February 24, 2021 09:23
@rhansen
Copy link
Collaborator Author

rhansen commented Feb 24, 2021

I added you as a contributor.

Thank you!

If you want I'm also happy to give you access to the npm package so you can publish new releases.

Yes please @felixge. My npm username is rhansen0.

@felixge
Copy link
Owner

felixge commented Feb 27, 2021

@rhansen I added you to npm, you should be able to publish new package versions now!

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 this pull request may close these issues.

2 participants