closed var changed too late #105
Conversation
Codecov Report@@ Coverage Diff @@
## master #105 +/- ##
==========================================
+ Coverage 66.18% 66.61% +0.43%
==========================================
Files 29 29
Lines 1384 1396 +12
==========================================
+ Hits 916 930 +14
+ Misses 468 466 -2
Continue to review full report at Codecov.
|
| if closed { return } | ||
| stopWatching() | ||
| if socket_close(self.descriptor) != 0 { | ||
| closed = true |
There was a problem hiding this comment.
Below we throw an error saying closeFailed so would setting it to true in the line above be dishonest about the current state of the socket?
|
I've tried to improve error handling |
Sources/SocksCore/TCPSocket.swift
Outdated
| stopWatching() | ||
| closed = true | ||
| if socket_close(self.descriptor) != 0 { | ||
| if errno != EBADF { |
There was a problem hiding this comment.
So basically, closed should be considered true unless it's a bad descriptor, EBADF? I think I'd prefer to set once instead of toggle incase there's some type of observing behavior at one point, maybe something like this
guard socket_close(self.descriptor) == 0 else {
// bad file descriptor EBADF should still be considered closed ... include reference here
if errno == EBADF { closed = true }
throw SocksError(.closeSocketFailed)
}
// set descriptor to -1 to prevent further use
self.descriptor = -1
closed = trueWhich brings up another question for me, can we get these two things
- a link to docs that show why
EBADFis still considered closed - on
EBADF, we're considering the socket, but not closing the descriptor w/ -1 and we're throwing. Should it still throw on this error, and should descriptor be set to -1 as well?
There was a problem hiding this comment.
closed var has to be set to true before trying to close the socket. Otherwise application checking closed var to send or receive data on the socket can be in inconsistent state. I've detected this error running benchmark with MongoKitten. Settings closed earlier prevent communication on closing socket. It's not clear for me how to handle the other error case. Obviously EBADF is clearly closed :-) so descriptor has to be set to -1.
…ror.socketIsClosed`
No description provided.