fs: fix ReadStream / WriteStream fd leaks #4405

Closed
wants to merge 2 commits into
from

Projects

None yet

2 participants

@bnoordhuis
Member

Fixes #4387. The PR is against master but it probably should go into v0.8.

Reviewer: @isaacs

@isaacs
isaacs commented Dec 11, 2012

Minor nit about setting this.fd = null, but otherwise LGTM. (If there's some reason not to do that, then that's probably fine, too; it's an edge case.)

bnoordhuis added some commits Dec 11, 2012
@bnoordhuis bnoordhuis fs: fix ReadStream fd leak
Close the file descriptor when a read operation fails.

Fixes #4387.
06ed12a
@bnoordhuis bnoordhuis fs: fix WriteStream fd leak
Close the file descriptor when a write operation fails.

Fixes #4387.
ce25575
@isaacs
isaacs commented on 06ed12a Dec 12, 2012

LGTM

@isaacs
isaacs commented on ce25575 Dec 12, 2012

LGTM.

@bnoordhuis
Member

Landed in v0.8 in 6e97b2c and d65832c.

@bnoordhuis bnoordhuis closed this Dec 12, 2012
@isaacs
isaacs commented Dec 15, 2012

@bnoordhuis Can you review 4791c32 and let me know if you think it's still a valid test for this issue?

The timing of events in fd streams is a bit different in streams2, so the test needed a bit of refactoring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment