Making GC of socket safer #399
Conversation
`fdSocket` is fairly unsafe, because the socket may be finalized (and therefore closed) while the file descriptor is still being used. `withFdSocket` offers a safer way to use the file descriptor.
|
Note that build history of CI shows that the bug has been fixed. |
|
@fumieval Would you review this? |
|
@Mistuke I'm now sure but should we do the same thing to |
fumieval
left a comment
fumieval
left a comment
There was a problem hiding this comment.
This commit looks fine otherwise.
tests/Network/SocketSpec.hs
Outdated
| _ <- forkIO gc | ||
| _ <- forkIO connect' | ||
| (_sock', addr) <- accept sock | ||
| -- check if an exception did not thrown. |
There was a problem hiding this comment.
Thanks. I will fix.
tests/Network/SocketSpec.hs
Outdated
| threadDelay 200000 | ||
| sock <- socket AF_INET Stream defaultProtocol | ||
| connect sock $ SockAddrInet 6000 $ tupleToHostAddress (127, 0, 0, 1) | ||
| it "can be GCed but not GCed when blocking" $ do |
There was a problem hiding this comment.
Does this test that it gets GC'd? I thought what we want to make sure is that it does not get GC'd.
There was a problem hiding this comment.
Historically, Socket cannot be GCed. The major feature of version 3 is that Socket can be GCed. So, Socket should be GCed in proper time. Anyway, I will consider how to rephrase it.
Network/Socket/Types.hsc
Outdated
| import GHC.Conc (closeFdWith) | ||
| import System.Posix.Types (Fd) | ||
| import Control.DeepSeq (NFData (..)) | ||
| import GHC.Weak (Weak (..)) |
There was a problem hiding this comment.
Hmm it looks like I forgot to fix redundant imports.
|
I added three commits based on @fumieval's comments. |
|
LGTM |
|
@kazu-yamamoto hmm the only usage of |
| threadDelay 200000 | ||
| sock <- socket AF_INET Stream defaultProtocol | ||
| connect sock $ SockAddrInet 6000 $ tupleToHostAddress (127, 0, 0, 1) | ||
| it "should not be GCed while blocking" $ do |
There was a problem hiding this comment.
Thanks for adding a test 🥇
|
Merged. Thank you, all guys, for your contribution! |
5 participants
Currently sockets are unintentionally GCed in non-loop code while API is blocked.
The first commit shows it.
This tends to happen where
fdSocketis used. For safety, the second commit introducedwithFdSocket.The third and fourth commits use
withFdSocketinstead offdSocket, resulting in passing the test.This takes over #398.