-
Notifications
You must be signed in to change notification settings - Fork 641
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
Release file handles back to caller on failure to take ownership #2715
Conversation
I can't think of a good way to unit test this. You can force a failure in the right place by passing in an already closed file handle as follows.
Using a closed file descriptor of course is almost certain to result in a flaky test, especially if run in parallel. |
f5ebf5d
to
c962e8b
Compare
@swift-server-bot test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The good news is that this is testable. The bad news is you need to use the Syscall Abstraction Layer to write the test. This will let you inject the EBADF you want to find, which will therefore also let you ensure the appropriate flow is hit.
Motivation: Allocation tests are slightly fragile to small changes. Modifications: Allow 50 extra allocations over the 1000 cycles. Result: Rst tests less flaky
Motivation: When taking ownership of input and output file descriptors in a bootstrap the ownership of the file handles remains with the caller in the function taking ownership fails. This is currently where _takingOwnershipOfDescriptors uses NIOFileHandle to take ownership of descriptors but then experiences a later failure. Modifications: If an exception is throw in _takingOwnershipOfDescriptors release file descriptors from NIOFileHandle. Result: No crash on failure to close file handles before end of scoped usage.
c962e8b
to
e5a1255
Compare
@Lukasa Let me know how you feel about the test I've tried adding. I looked into hooking sys calls but I believe the current sys call hooking doesn't touch this area at all yet. In order to hook the call made directly from PipeChannel I think I'd need to add a hooking mechanism to that which would involved changing the construction of that. Given the failure is from the construction of PipeChannel feels simpler to just throw the exception directly from a hooking factory for that. Interested to hear your thoughts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't hate this testing strategy, but it does need a little cleanup.
Co-authored-by: Cory Benfield <[email protected]>
* commit 'fc79798d5a150d61361a27ce0c51169b889e23de': NIOSendableBox: allow off-loop initialisation iff Value is Sendable (apple#2753) Throw an appropriate error from the writer when the channel closed (apple#2744) put snippet code inside @available function (apple#2750) fix link to NIOFileSystem from NIO index page (apple#2747) convert the NIOFileSystem example code to a Snippet (apple#2746) Silence warning about missing include in macOS builds (apple#2741) Correctly mark 304 as not having a response body (apple#2737) Update availability guard (apple#2739) Add API for setting last accessed and last modified file times (apple#2735) Add a fallback path if renameat2 fails (apple#2733) Release file handles back to caller on failure to take ownership (apple#2715) Add a version of 'write' for 'ByteBuffer' (apple#2730) Imrprove rename error (apple#2731) Remove storage indirection for FileSystemError (apple#2726) testSimpleMPTCP should not fail for ENOPROTOOPT (apple#2725) Fix race in TCPThroughputBenchmark (apple#2724)
Motivation:
When taking ownership of input and output file descriptors in a bootstrap the ownership of the file handles remains with the caller in the function taking ownership fails. This is currently where _takingOwnershipOfDescriptors uses NIOFileHandle to take ownership of descriptors but then experiences a later failure.
Modifications:
If an exception is throw in _takingOwnershipOfDescriptors release file descriptors from NIOFileHandle.
Result:
No crash on failure to close file handles before end of scoped usage.