Close prime #337

kazu-yamamoto · 2018-06-27T01:40:06Z

This should fix #329 and #335.

kazu-yamamoto · 2018-06-27T01:41:11Z

Cc: @winterland1989, @vdukhovni

vdukhovni · 2018-06-27T01:49:59Z

Network/Socket/Types.hsc

+--
+--   This function is not thread-safe. Consider the following senario.
+--
+--   1) Thread A acquires a 'Fd' from 'Socket' by 'fdSocket'.


I don't think this is a thread-safety issue with close. Frankly, the issue here is with fdSocket, not close. If you acquire an fdSocket it can stop being valid at any time if other threads manipulate the same Socket.

I agree with @vdukhovni.
@winterland1989 Do you mind if I move this doc to fdSocket?

Is there any other operations that will invalidate socket other than close?

Yes. socketToHandle

Invalidation is not a thread safety issue. If you wanted to make socketFd safer, it would dup(2) the socket, so that the returned file descriptor would remain valid even if the original socket is closed elsewhere. But since a bare file descriptor has no finalizer, the right thing to provide is a clone operation that wraps a socket with a finalizer around a duplicate socket, and then a thread can get a "private" copy of a socket, on which it can call "fdSocket" if that's needed for some reason (rather than using the wrapped socket for all I/O).

Bottom line, close is a fine thread-safe operation, what is not semantically thread-safe is using the same file descriptor in multiple threads, with any of them free to close the descriptor. EVen without closing it is very difficult to ensure that I/O is properly interleaved. Using the same socket in multiple threads is not a common design pattern.

Well, my purpose is to stop user calling close from another thread, which is common in some timeout code. I'm not particularly cared other use cases since socket is stream based transport layer rather than message based which doesn't make sense when used under multiple threads.

To understand this well, would you show a typical example where close is used in timeout code?

By using the timer manager from base on unix:

import GHC.Event ... tm <- getSystemTimerManager registerTimeout tm 1000000 (close socketA) ...

Or use threadDelay on windows:

forkIO $ threadDelay 1000000 >> close socketA

Here close will be called either from the timer manager thread, or a thread created by user. You can argue that the right thing to do here is to use timeout from System.Timeout which use async exception, but what i'd like to do here is to make people aware that above code is unsafe.

This is a bad application design, but is not a thread-safety issue in close. The close function will not close the wrong file or crash. It behaves correctly when run in multiple threads.

You don't get correct semantics when you close sockets that are being used by a running thread, but that's using the same socket in two threads, and that's generally a problem.

When two threads race to bind, listen or connect the same socket one fails. When two threads race to read the same stream socket they may each see fragments of application level messages (or intersperse fragments of output messages when writing).
One can make a general comment that use of the same socket in multiple threads is unsafe except as noted.

I'd expect accept to be thread-safe, with each thread getting a separate client connection from the same listening socket. And if some sort of "clone" operation were provided, it could provide some measure of safety against close in another thread, but would still be subject to races in send or recv.

Because we don't want to hold any type of lock around a socket while performing I/O operations that can block, such operations look up the file descriptor atomically (at least in 3.0), but then use the descriptor even though it may become invalid, or another thread may perform interleaved operations. This is a basic fact about external handles, and there's no implementation-specific thread-safety caveat here. Don't mess with the the state of a single socket from multiple threads.

There is one pattern that is safe. One thread reads and a different thread writes, but here I'd be tempted to clone the socket so that the reader and writer have separate underlying file descriptors . Of course if the application waits for both threads and then closes the socket when both are done, that's OK too.

I made a pointer from close's doc to fdSocket's doc.
@winterland1989 Is this acceptable for you?

vdukhovni · 2018-06-27T01:53:00Z

Network/Socket/Types.hsc

 close :: Socket -> IO ()
 close s = invalidateSocket s (\_ -> return ()) $ \oldfd -> do
-    closeFdWith (void . c_close . fromIntegral) (fromIntegral oldfd)
+    -- closeFdWith avoids the deadlock of IO manager.
+    closeFdWith closeFd (toFd oldfd) `E.catch` ignore


Does this also catch asynchronous exceptions? It would be better if we had a low-level close that returned an "int", and throw an exception in close' and not throw in close, rather than try to catch exceptions here.

That is use c_close here and ignore the return value!

That is use c_close here and ignore the return value!

Ah. You are right. c_close does not throw exceptions!

Does this also catch asynchronous exceptions?

What kind of scenario do you think?

Things like timeouts and thread cancellation are asynchronous timeouts and must not be caught here. Michael Snoyman has a Control.Exception.Safe module that can catch just synchronous exceptions, but there's no need for that here...

http://hackage.haskell.org/package/safe-exceptions-0.1.7.0

ignore was removed.
We should consider asynchronous exceptions yet, if necessary.

OK. I understand what you want to explain. I hope that you like the current code.

vdukhovni · 2018-06-27T05:34:10Z

Network/Socket/Types.hsc

+    closeFd = void . c_close . fromIntegral
+
+-- | Close the socket. This function throws exceptions if
+--   the underlining system call returns errors.


s/underlining/underlying/

vdukhovni · 2018-06-27T05:34:30Z

Network/Socket/Types.hsc

@@ -112,11 +116,53 @@ invalidateSocket (Socket ref _) errorAction normalAction = do

 -----------------------------------------------------------------------------

-- | Close the socket. Sending data to or receiving data from closed socket
+-- | Close the socket. This function does not throw exceptions even if
+--   the underlining system call returns errors.


s/underlining/underlying/

vdukhovni

Looks OK to me, module spelling corrections.

vdukhovni · 2018-06-28T00:37:58Z

Network/Socket/Types.hsc

+--
+--   If multiple threads use the same socket and one uses 'fdSocket' and
+--   the other use 'close', unexpected behavior may happen.
+--   For more information, please refer to the documentation of 'fdSocket'.


The issue affects more than just fdSocket and more than just close. In general it is semantically unsafe for two threads to use the same socket, data may be corrupted, or sent to or read from the wrong place. The only exceptions I can think of are:

A reader thread can coëxist with a writer thread

Multiple threads can call accept to process incoming connections

So I think this sort of advice belongs at the top of the package, not just the close function.

Thanks. I will.

yes, that will be nice.

vdukhovni · 2018-06-29T14:16:38Z

Network/Socket/Types.hsc

+--      number which thread A is holding.
+--
+--   In this case, it is safer for Thread A to clone 'Fd' by
+--   'System.Posix.IO.dup'.


Note, that even dup is not safe if the socket is not locked against close between fdSocket and dup. For a safe dup we'd need to provide an atomic variant, that is guaranteed to be duping the unclosed descriptor inside the given socket. You'd need an MVar associated with the socket to implement a safe dup.

The solution is to avoid use of the same socket (other than acceptor or separation of read/write) in multiple threads. All other concurrency models are unsound, and no thread may manipulate a socket concurrently with another thread outside the limited exception cases.

I will add this race condition to the doc.

kazu-yamamoto added 7 commits June 26, 2018 16:55

"close" ignores exceptions.

fdf2afa

implementing close'.

b7cb831

updating doc of "close".

cd26f14

adding "not thread-safe" explanation to "close". (haskell#335)

873f1c1

updating doc.

b0f8e64

refactoring.

48ba263

fixing a typo.

b843be3

vdukhovni reviewed Jun 27, 2018

View reviewed changes

removing exception stuff.

2e9d9b7

vdukhovni reviewed Jun 27, 2018

View reviewed changes

vdukhovni approved these changes Jun 27, 2018

View reviewed changes

kazu-yamamoto added 3 commits June 27, 2018 15:57

fixing typos.

435f16b

updating doc.

b94abf2

updating doc.

e8fa6ff

vdukhovni reviewed Jun 28, 2018

View reviewed changes

kazu-yamamoto merged commit 30b7d79 into haskell:master Jun 29, 2018

kazu-yamamoto deleted the close-prime branch June 29, 2018 04:25

vdukhovni reviewed Jun 29, 2018

View reviewed changes

eborden mentioned this pull request Jul 7, 2018

Releasing 2.7.0.2 #332

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Close prime #337

Close prime #337

kazu-yamamoto commented Jun 27, 2018

kazu-yamamoto commented Jun 27, 2018

vdukhovni Jun 27, 2018

kazu-yamamoto Jun 27, 2018

winterland1989 Jun 27, 2018

kazu-yamamoto Jun 27, 2018

vdukhovni Jun 27, 2018

winterland1989 Jun 27, 2018

kazu-yamamoto Jun 27, 2018

winterland1989 Jun 27, 2018

vdukhovni Jun 27, 2018

kazu-yamamoto Jun 28, 2018

vdukhovni Jun 27, 2018

vdukhovni Jun 27, 2018

kazu-yamamoto Jun 27, 2018

kazu-yamamoto Jun 27, 2018

vdukhovni Jun 27, 2018

kazu-yamamoto Jun 27, 2018

kazu-yamamoto Jun 27, 2018

vdukhovni Jun 27, 2018

vdukhovni Jun 27, 2018

vdukhovni left a comment

vdukhovni Jun 28, 2018

kazu-yamamoto Jun 29, 2018

winterland1989 Jun 29, 2018

vdukhovni Jun 29, 2018

kazu-yamamoto Jul 2, 2018

Close prime #337

Close prime #337

Conversation

kazu-yamamoto commented Jun 27, 2018

kazu-yamamoto commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdukhovni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment