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

Close file descriptor on drop in TimerFd #1381

Merged
merged 2 commits into from Feb 20, 2021

Conversation

crdumoul
Copy link

@crdumoul crdumoul commented Feb 1, 2021

This change closes the TimerFd file descriptor on drop. Note that the TimerFd will no longer be Clone or Copy. Since it has a destructor it can't be Copy, and if it were Clone you could end up trying to use a closed TimerFd, or double-closing the file descriptor.

Addresses #1379.

Copy link
Member

@asomers asomers left a comment

Good catch about Clone. I think there may be some other structs that have that problem; I'll check for them.

A few requests:

  • The drop implementation should panic on EBADF, because that usually indicates a double-close.
  • Document somewhere that this type closes on drop.
  • Don't forget to add a CHANGELOG entry!

@crdumoul
Copy link
Author

crdumoul commented Feb 3, 2021

I believe I've addressed the suggested changes.

CHANGELOG.md Outdated
@@ -10,6 +10,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- Added limited Fuchsia support (#[1285](https://github.com/nix-rust/nix/pull/1285))
- Added `getpeereid` (#[1342](https://github.com/nix-rust/nix/pull/1342))
### Fixed
- TimerFd now closes the underlying fd on drop.
Copy link
Member

@asomers asomers Feb 7, 2021

Choose a reason for hiding this comment

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

Also, in the "Removed" section you should note that is no longer Clone and Copy.

Copy link
Author

@crdumoul crdumoul Feb 8, 2021

Choose a reason for hiding this comment

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

Done

libc::close(self.fd)
});
if let Err(Error::Sys(Errno::EBADF)) = result {
panic!("close of TimerFd encountered EBADF");
Copy link
Member

@asomers asomers Feb 7, 2021

Choose a reason for hiding this comment

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

One problem: panicking during drop is usually bad, because drop itself gets called during panic. A double-panic is difficult to debug. Instead, you should only panic if the thread isn't already panicking. In fact, you don't even need to close when panicking, because the process is about to abort anyway. So I would write this as

if !std::thread::panicking() {
    //close the file descriptor, panicking on EBADF
}

Copy link
Author

@crdumoul crdumoul Feb 8, 2021

Choose a reason for hiding this comment

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

The file descriptor is now only closed if the thread isn't already panicking.

@asomers
Copy link
Member

asomers commented Feb 13, 2021

Would you mind squashing your commits? We can't do this with Github's merge button because we use bors.

@crdumoul
Copy link
Author

crdumoul commented Feb 16, 2021

Would you mind squashing your commits? We can't do this with Github's merge button because we use bors.

This is done.

Copy link
Member

@asomers asomers left a comment

bors r+

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.

None yet

2 participants