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

don't leak file descriptors <3 #22678

Closed
wants to merge 1 commit into from
Closed

don't leak file descriptors <3 #22678

wants to merge 1 commit into from

Conversation

mahkoh
Copy link
Contributor

@mahkoh mahkoh commented Feb 22, 2015

Don't merge before old_io has been removed.

cc @alexcrichton @aturon

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@mahkoh
Copy link
Contributor Author

mahkoh commented Feb 22, 2015

Fixes #19028

// Note that errors are ignored when closing a file descriptor. The reason for
// this is that if an error occurs we don't actually know if the file descriptor
// was closed or not, and if we retried (for something like EINTR), we might close
// another valid file descriptor (opened after we closed ours.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add the missing ) while you're here?

@alexcrichton
Copy link
Member

While I agree that we should land this soon, I do not believe that we can do so currently due to the reliance of old_io. I believe, for example, that the test suite does not pass if this is landed.

In the interest of keeping the queue relatively clear I'm going to close this for now but we can certainly land once old_io is dealt with.

@l0kod
Copy link
Contributor

l0kod commented Mar 1, 2015

I hope #22797 can help to merge this PR.

@l0kod
Copy link
Contributor

l0kod commented Mar 11, 2015

cc #12148

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.

6 participants