Repository navigation
GCing Sockets is a bad diea #302
Description
Activity
Was there really a version 2.6.3.4? I don't see it anywhere... If it had no finalizers defined, its close API was still OK, and safe for repeated calls.
See #300.
My proposal is release 2.6.3.4, 2.7.0.0 and 3.0.0.0 in order. If @eborden agree with this, we will carry out this plan.
- version 2.6.3.4 #300 is for 2.6.3.4
- https://github.com/kazu-yamamoto/network/tree/poc-2.7.0.0 for 2.7.0.0 (We will override the current
2.7branch if we reach consensus.) masteris for 3.0.0.0
See also #296.
@vdukhovni Sorry for your confusion. My plan is to override
2.7with https://github.com/kazu-yamamoto/network/tree/poc-2.7.0.02.7removedSockAddrCanbut we need to revert it for smooth migration. Unfortunately, it appeared that it is hard to revertSockAddrCan. That's why I createdpoc-2.7.0.0based on2.6.To be honest I am not sure that removing the
mStatfield of Socket is the right decision. It originally made it possible to track whether the socket is already closed or not (or converted to a handle, ...) and thus instead of simply asking users to not make certain "misuse" mistakes, made it possible to detect such misuse.If performance is not impacted, I'd even consider storing the file descriptor in an IORef, and replace it with
-1on close. In my view, Haskell is supposed to be firstly safer than C, and only then reasonably fast. So exposing unsafe interfaces (thereby enabling double-close errors, ...) may not be worth any performance gains...For
SocketStatusstuff, please read #286 (comment)Socketin v3.0.0.0 is an abstract type. So, we can hold any fields if we wish.IORefmight be a good idea.After releasing 2.7.0.0, we should wait for a long time so that users of network library stop using deprecated APIs. So, we have enough time. Let's continue discussion.
For example,
Hadledetects and suppresses duplicatecloseoperations. When I compile and trace:module Main where import System.IO main :: IO () main = do n <- openBinaryFile "/dev/null" ReadMode hClose n hClose n hClose nI see just one close:
openat(AT_FDCWD,"/dev/null",O_RDONLY|O_NONBLOCK|O_NOCTTY,00) = 4 (0x4) fstat(4,{ mode=crw-rw-rw- ,inode=27,size=0,blksize=4096 }) = 0 (0x0) ioctl(4,TIOCGETA,0x7fffffffa3f0) ERR#25 'Inappropriate ioctl for device' close(4) = 0 (0x0) clock_gettime(15,{ 0.005872000 }) = 0 (0x0) ioctl(1,TIOCGETA,0x7fffffffa3b0) ERR#25 'Inappropriate ioctl for device' clock_gettime(15,{ 0.005966000 }) = 0 (0x0) clock_gettime(15,{ 0.006341000 }) = 0 (0x0) exit(0x0)Likely
Network.Socketshould have similar safety built-in.Your requirement is:
- a
Socketclosed by Haskell'sclosemust not be reused - Haskell's
closemust not throw an error even if theSocketis already closed
Right?
- a
It's worth confirming that
Socket (IORef (-1))can be GCed naturally.@kazu-yamamoto Your requirement is:
a Socket closed by Haskell's close must not be reusedNo, rather a Socket that is still reachable must remember that it is already closed, so that a second close will not close the
same(but not really the same anymore) file descriptor.Haskell's close must not throw an error even if the Socket is already closed
Actually it is OK to throw an error, but perhaps not needed (System.IO handles seem to not bother) just don't do it in a finalizer. If you throw on close, then also provide some sort of
_closethat does not throw for use in finalization.Right?
I guess not, but is the above more clear?
@kazu-yamamoto It's worth confirming that Socket (IORef (-1)) can be GCed naturally.
Of course it GC's just fine, there's no
foreigncontent here or anything else to stop GC. What's important here is that a socket whose mutable file descriptor can be updated to-1(modulo multiple threads racing to close the same object) is safe to close multiple times or close again in finalization. Just read the FD from the IORef, and close if not already-1on Unix orINVALID_SOCKETon Windows (use some platform specific compile-time constant for this), then store the invalid value back into the IORef. Concurrent races to close the socket are unsafe, but IIRC this was also unsafe even with the legacymStatbecause the check and replace were not atomic. Multiple threads trying to close the same socket is much less common than multiple close within the thread that "owns" the socket, or when finalizing or ResourceT cleanup, ...If we are to utilize an
IORefwe'll need to usemkWeakIORefto init the finalizer. It also seem completely reasonable to signal a closed socket withIORef (-1).@eborden I don't understand why
mkWeakIORefis necessary. The idea here is that we should not set a finalizer to Socket to close it.@kazu-yamamoto I don't understand why mkWeakIORef is necessary. The idea here is that we should not set a finalizer to Socket to close it.
Well, actually, if the Socket does contain a mutable IORef with the file-descriptor inside it, then you can ensure that
closehappens at most once, in which case you might consider adding finalization, unless that's too much overhead, and users close most sockets directly or via ResourceT or similar, so perhaps it would turn out to be an unnecessary cost. I don't know whether automatic finalization is useful or not, but it would no longer be unsafe.Here's a small demo of the possible finalization approach. (Whether finalization is a good idea is still an open question in my mind, perhaps most applications don't need this, and perhaps the cost of adding it is noticeable? But if it is cheap enough, then perhaps worth it, for those applications that neglect to clean up).
module Main (main) where import Data.IORef import System.Mem import Control.Monad (void) myClose :: IORef Int -> IO () myClose r = do old <- atomicModifyIORef' r $ \old -> (-1, old) case old of -1 -> return () _ -> putStrLn $ "Closing: r" ++ show old main :: IO () main = do r1 <- newIORef 1 r2 <- newIORef 2 r3 <- newIORef 3 void $ mkWeakIORef r1 $ myClose r1 void $ mkWeakIORef r2 $ myClose r2 void $ mkWeakIORef r3 $ myClose r3 myClose r2 myClose r3 myClose r3 performGC
Running it shows exactly one close for each of the three objects, one closed when finalized, other two closed explicitly by the application, one twice, but all three do the (just print a message) close processing just once.
./foo Closing: r2 Closing: r3 Closing: r1@vdukhovni Thanks! I think that I understand now.
#303 has been merged. Let's close this.
Cc: @vdukhovni
We are planning to add a finalizer to
Sockets so that they can be GCed.GHC's bug 1
In network v2.6.3.4, we use:
The definition of
closeis:So, if a
Socketis converted intoHandleand theSocketbecomes unreach,closeis called and an error is thrown. We believed that this is just ignored. Unfortunately, this is not the case. GHC has a bug. If a finalizer throws an error, other finalizers are not called:https://ghc.haskell.org/trac/ghc/ticket/13167
GHC's bug 2
For 3.0.0.0, we are trying to use
addFinalizer: #301As I already reported, GHC 7.10 GCes
Sockets too early. I also found that GHC 7.8 has the same bug.Bad usage
A user can close the file descriptor of
Socketby calling theclosesyscall. Then the user opens another file or socket and the number is reused. In this situation, calling finalizer by GC results in unexpected bug.Proposed actions
mkWeakMVarfor v2.6.3.4Socketsare not GCed and users SHOULD close them by themselves.