Safer socket #336
Safer socket #336
Conversation
| s <- mkSocket fd | ||
| unsetIPv6Only s `E.onException` close s | ||
| return s | ||
| where |
There was a problem hiding this comment.
I like the improvement in readability of moving the ifdefs to where.
| c_socket (packFamily family) c_stype protocol | ||
| setNonBlock fd `E.onException` c_close fd | ||
| s <- mkSocket fd | ||
| unsetIPv6Only s `E.onException` close s |
There was a problem hiding this comment.
Should these be closing and throwing? Otherwise socket will return a Socket that has already been closed and file descriptor re-use by the OS could lead to us accidentally sending to an unknown socket.
There was a problem hiding this comment.
Actually reuse is not an issue here since close changes it to -1, but is above with using vanilla c_close.
| c_stype <- modifyFlag <$> packSocketTypeOrThrow "socket" stype | ||
| fd <- throwSocketErrorIfMinus1Retry "Network.Socket.socket" $ | ||
| c_socket (packFamily family) c_stype protocol | ||
| setNonBlock fd `E.onException` c_close fd |
There was a problem hiding this comment.
Won't this result in a Socket with a closed file descriptor within its IORef?
eborden
left a comment
eborden
left a comment
There was a problem hiding this comment.
Woops, ignore the issues above. onException is fine here, since it isn't catching anything, but just a flavor of bracket.
eborden
left a comment
eborden
left a comment
There was a problem hiding this comment.
Actually I'm not sure. onException is defined as:
onException :: IO a -> IO b -> IO a
onException io what = io `catch` \e -> do _ <- what
throwIO (e :: SomeException)
Is that sufficient to prevent async exceptions from causing file descriptors leaking? It seems we might want to utilize bracketOnError to ensure masking and do something like:
bracketOnError create c_close $ \fd -> do
setNonBlock fd
s <- mkSocket fd
unsetIPv6Only s
return s
where
create =
c_stype <- modifyFlag <$> packSocketTypeOrThrow "socket" stype
throwSocketErrorIfMinus1Retry "Network.Socket.socket" $
c_socket (packFamily family) c_stype protocol
|
@eborden My code is safe only for synchronous exceptions while your code is safe even for asynchronous exceptions! Also, OK. I will adopt your code. Thank you for your suggestion! |
|
Rebased and merged. |
2 participants
socketcloses the file descriptor on exception.This should fix #166 in
master.