apple / swift-nio

Event-driven network application framework for high performance protocol servers & clients, non-blocking.
https://swiftpackageindex.com/apple/swift-nio/documentation
Apache License 2.0
7.85k stars 633 forks source link

Release file handles back to caller on failure to take ownership #2715

Closed PeterAdams-A closed 1 month ago

PeterAdams-A commented 1 month ago

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.

PeterAdams-A commented 1 month ago

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.

let sock = socket(AF_LOCAL, SOCK_STREAM, 0)
close(sock)

let channelFuture = NIOPipeBootstrap(group: NIOSingletons.posixEventLoopGroup).takingOwnershipOfDescriptor(inputOutput: sock)

Using a closed file descriptor of course is almost certain to result in a flaky test, especially if run in parallel. Using numbers of file descriptors which are out of range or have never been used causes failure earlier in the call stack.

PeterAdams-A commented 1 month ago

@swift-server-bot test this please

PeterAdams-A commented 1 month ago

@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.