IO: Polish IO closing for MT #8733
Conversation
|
I'd at least like a better name... |
|
The lock in thread local value did not need to be exposed until now at least. I agree the name is not the best, but it's private api and is only used in the lines that were changed in this PR. I see it more like a helper for the implementation. Another name could be |
|
I'd prefer |
ThreadLocalValue#each and ThreadLocalValue#clear were requesting a lock separately. If two threads were closing the IO the event could be freed twice.
bcardiff
force-pushed
the
mt/io-close
branch
from
08cfa24
to
e6ace56
Compare
|
Rebased on master and method renamed. Approval pending. |
| @@ -6,7 +6,11 @@ module Crystal::System::FileDescriptor | |||
| private def unbuffered_read(slice : Bytes) | |||
| bytes_read = LibC._read(@fd, slice, slice.size) | |||
| if bytes_read == -1 | |||
| raise Errno.new("Error reading file") | |||
| if Errno.value == Errno::EBADF | |||
There was a problem hiding this comment.
File descriptors can be reused - if this if branch can be hit, then doesn't that imply that a write may happen to the wrong file descriptor because of a race condition?
There was a problem hiding this comment.
I'm not sure how probably that reutilization.
The same check was added to #unbuffered_write in #6497.
Also with waj we mention that these checks might need to move to evented directly.
There was a problem hiding this comment.
I'm not sure how probably that reutilization
Well they're sequential, so very probably.
If you open, close, open again, the second open will have exactly the same FD as the first. There must be no point after LibC.close is called that the FD is used again in another syscall (for the same IO::FileDescriptor, there's not much we can do if there exists two objects).
There was a problem hiding this comment.
Yeah looks like a possible race condition, tricky one... Appears that java avoids this by setting the internal "fd" to -1 before doing a close: https://github.com/openjdk-mirror/jdk7u-jdk/blob/f4d80957e89a19a29bb9f9807d2a28351ed7f7df/src/solaris/native/java/io/io_util_md.c#L93 they also synchronize the calls to close FWIW: https://github.com/openjdk-mirror/jdk7u-jdk/blob/master/src/share/classes/java/io/FileInputStream.java#L289
For now IO#close is not thread-safe. You should not close the IO from concurrently.
Even though if that's the case, before this PR the LibEvent events could be freed twice and the awaiting readers/writers fibers could be enqueued again. This is now fixed by the changes in
io/evented.cr.I also align the exception raised when the awaiting reader is awakened with a EBADF / closed fd.
The
IO::Erroradded is to match the description when a writer is awakened with a EBADF. These does not match 100% the error if the fd was closed from the beginning. This bugs me a little.Also the
socket.crdoes not have this special handling of Errno inunbuffered_read/unbuffered_write.The exact exception type and message might come back with Errno exceptions are revisited.
After this PR if
IO#closeis called concurrently from multiple threads one might fail withErrno.new("Error closing file")inCrystal::System::FileDescriptor#file_descriptor_closesince the fd might be already closed. Without adding a lock forIO#closeit is not possible to prevent this situation. The implementation ofevented_closeit is thread-safe now and that protects the runtime enough.