Not retry calling close(...) when EINTR is reported. #217
Conversation
normanmaurer
force-pushed
the
close_eintr
branch
2 times, most recently
from
71b8ffa to
43fc0e0
Compare
|
Thanks to @vlm for reporting. |
|
@swift-nio-bot test this please |
Sources/NIO/System.swift
Outdated
| /* Sorry, we really try hard to not use underscored attributes. In this case however we seem to break the inlining threshold which makes a system call take twice the time, ie. we need this exception. */ | ||
| @inline(__always) | ||
| internal func wrapSyscall<T: FixedWidthInteger>(where function: StaticString = #function, _ body: () throws -> T) throws -> T { | ||
| internal func wrapSyscall<T: FixedWidthInteger>(where function: StaticString = #function, continueOnErrno: (CInt) -> Bool = continueOnEINTR, _ body: () throws -> T) throws -> T { |
There was a problem hiding this comment.
Should we just make this a separate function rather than use the closure?
There was a problem hiding this comment.
I'm a bit reluctant to combine @inline(__always) with this closure, that's all.
There was a problem hiding this comment.
ok let me just add an extra method then... Its easy enough to do :)
Motivation: If close(...) reports EINTR there is nothing sane we can do so it makes no sense to even report it. See also: apple/swift-nio#217 Modifications: Just ignore EINTR when calling close(...) Result: Less noise in the logs.
normanmaurer
force-pushed
the
close_eintr
branch
from
43fc0e0 to
c41f8ac
Compare
Sources/NIO/System.swift
Outdated
| throw IOError(errnoCode: err, function: function) | ||
| } | ||
| return res | ||
| } |
There was a problem hiding this comment.
In this case don't we just want to suppress the return of EINTR altogether? I don't think we need to throw at all.
There was a problem hiding this comment.
works for me as well...
normanmaurer
force-pushed
the
close_eintr
branch
from
c41f8ac to
f5a8b85
Compare
|
@weissi check the links that are included in the comments... Basically there is really nothing we can do about it and the "safest" bet is to assume it worked. |
|
@swift-nio-bot test this please |
|
@swift-nio-bot test this please |
Motivation: Usually we retry the syscall when EINTR is reported. This is not safe in the case of close(...) as the file descriptor may already be closed. The problem arise if this was the case and we retry it and the file descriptor is already reused again. In this case we would close a wrong file descriptor. See: - https://bugs.chromium.org/p/chromium/issues/detail?id=269623 - https://lwn.net/Articles/576478/ Modifications: Ignore EINTR on close(...) and just assume the FD was closed. Result: Not possible to close the wrong file descriptor when close(...) reports EINTR.
normanmaurer
force-pushed
the
close_eintr
branch
from
42fd4b0 to
ad81425
Compare
|
@Lukasa it should actually be quite possible to trigger it:
I think this should be enough to reliably observe EINTR. If not, the whole thing should be tried with an NFS instead of sockets, but that complicates stuff greatly. |
|
No, turned out under Linux it is pretty impossible to get it this way. Take it back. |
Motivation: If close(...) reports EINTR there is nothing sane we can do so it makes no sense to even report it. See also: apple/swift-nio#217 Modifications: Just ignore EINTR when calling close(...) Result: Less noise in the logs.
Motivation:
Usually we retry the syscall when EINTR is reported. This is not safe in the case of close(...) as the file descriptor may already be closed. The problem arise if this was the case and we retry it and the file descriptor is already reused again. In this case we would close a wrong file descriptor.
See:
Modifications:
Ignore EINTR on close(...) and just assume the FD was closed.
Result:
Not possible to close the wrong file descriptor when close(...) reports EINTR.