GCing Sockets is a bad diea

[kazu-yamamoto]

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:

   _ <- mkWeakMVar mStat $ close sock

The definition of close is:

close (MkSocket s _ _ _ socketStatus) = do
 modifyMVar_ socketStatus $ \ status ->
   case status of
     ConvertedToHandle ->
         ioError (userError ("close: converted to a Handle, use hClose instead")
)
     Closed ->
         return status
     _ -> closeFdWith (closeFd . fromIntegral) (fromIntegral s) >> return Closed

So, if a Socket is converted into Handle and the Socket becomes unreach, close is 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: #301

As 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 Socket by calling the close syscall. 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

• Reject GCing Socket in the new definition. #301 for v3.0.0.0
• Revert mkWeakMVar for v2.6.3.4
• Write documentation that Sockets are not GCed and users SHOULD close them by themselves.

[vdukhovni]

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.

[kazu-yamamoto]

See #300.

[vdukhovni]

But I do see #269, so I guess that was it...

[kazu-yamamoto]

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.7 branch if we reach consensus.)
• master is for 3.0.0.0

See also #296.

[kazu-yamamoto]

@vdukhovni Sorry for your confusion. My plan is to override 2.7 with https://github.com/kazu-yamamoto/network/tree/poc-2.7.0.0

2.7 removed SockAddrCan but we need to revert it for smooth migration. Unfortunately, it appeared that it is hard to revert SockAddrCan. That's why I created poc-2.7.0.0 based on 2.6.

[vdukhovni]

To be honest I am not sure that removing the mStat field 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 -1 on 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...

[kazu-yamamoto]

For SocketStatus stuff, please read #286 (comment)

[kazu-yamamoto]

Socket in v3.0.0.0 is an abstract type. So, we can hold any fields if we wish. IORef might 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.

[vdukhovni]

For example, Hadle detects and suppresses duplicate close operations. 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 n

I 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.Socket should have similar safety built-in.

[kazu-yamamoto]

Your requirement is:

• aSocket closed by Haskell's close must not be reused
• Haskell's close must not throw an error even if the Socket is already closed

Right?

[kazu-yamamoto]

It's worth confirming that Socket (IORef (-1)) can be GCed naturally.

[vdukhovni]

@kazu-yamamoto Your requirement is:
a Socket closed by Haskell's close must not be reused

No, 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 _close that does not throw for use in finalization.

Right?

I guess not, but is the above more clear?

[vdukhovni]

@kazu-yamamoto It's worth confirming that Socket (IORef (-1)) can be GCed naturally.

Of course it GC's just fine, there's no foreign content 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 -1 on Unix or INVALID_SOCKET on 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 legacy mStat because 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, ...

[eborden]

If we are to utilize an IORef we'll need to use mkWeakIORef to init the finalizer. It also seem completely reasonable to signal a closed socket with IORef (-1).

[kazu-yamamoto]

@eborden I don't understand why mkWeakIORef is necessary. The idea here is that we should not set a finalizer to Socket to close it.

[vdukhovni]

@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 close happens 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.

[vdukhovni]

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

[kazu-yamamoto]

@vdukhovni Thanks! I think that I understand now.

[kazu-yamamoto]

#303 has been merged. Let's close this.