Add unsafeFdSocket and touchSocket (Fixes #418)

[takano-akio]

This branch implements unsafeFdSocket and touchSocket as discussed in #418.

Before making the actual change, I had to update/move the warning about race condition in the doc for fdSocket because (if I understand correctly) it was very misleading. Basically the race condition is not specific to fdSocket at all, and it applies to any combination of close and a normal socket operation (like recv). Please review the doc update carefully because it's a tricky topic and I may well have made mistakes.

[kazu-yamamoto]

Basically the race condition is not specific to fdSocket at all, and it applies to any combination of close and a normal socket operation (like recv).

I believe this is not correct. It's specific to fdSocket, not to recv. The operations via Socket including recv is safe because its internal file descriptor is replaced with (-1) by close.

[takano-akio]

It seems to me that the following sequence of events could happen.

1. Thread A calls recvBuf, which then calls withFdSocket, which then reads the FD from the IORef.
2. Thread B calls close, which updates the IORef with (-1).
3. Thread B calls the close system call.
4. Thread A calls the recv system call. This is bad because it's an operation on a closed (and potentially reused) file descriptor.

I believe some kind of locking is necessary to prevent this race condition.

[kazu-yamamoto]

@vdukhovni Akio pointed out a possible race condition for the current code. The IORef idea came from you. What do you think of this case?

[vdukhovni]

@vdukhovni Akio pointed out a possible race condition for the current code. The IORef idea came from you. What do you think of this case?

The close call in thread B is incorrect (an application bug). There is no reasonable way to use locks here directly. The best one can do is also keep a reference count and use locks to atomically increment and decrement the reference count, with any thread that decrements the count to zero then closing the socket, and close being essentially a decrement operation, with a possible side-effect.

But I think this is too expensive, and generally unnecessary. What've implemented is safe finalization, which coexists with "prompt" resource deallocation via explicit close, without promising safety for races between multiple threads using the same socket.

I think that's enough.

[kazu-yamamoto]

I agree with @vdukhovni. It's application's bug.
@takano-akio What do you think?

[vdukhovni]

In terms of documentation though, yes any use of close is unsafe while some other thread is using the socket, or has called fdSocket with the intention of using the fd directly. The caller of close must be the last thread to be using the socket.

[vdukhovni]

This probably needs an example showing the function is used in a code block that keeps the socket alive...

[vdukhovni]

Putting aside the question of locking, and supporting concurrent close, this PR makes it possible to use fdSocket more safely by adding a call to touchSocket at the end of the code block that uses the file descriptor. So the PR should probably be merged, but the documentation changes likely need a bit more polish.

[kazu-yamamoto]

I would like to divide this issue to three items:

1. Adding unsafeFdSocket and touchSocket
2. Improving documentation
3. Considering socketToFd

I will take care of (1) first.

[kazu-yamamoto]

For (1), I would like to merge #423 instead of #422 (this one).
@takano-akio @vdukhovni Do you agree?

[vdukhovni]

For (1), I would like to merge #423 instead of #422 (this one).
@takano-akio @vdukhovni Do you agree?

Largely, yes, but I see that #423 has touchFdSocket rather than touchSocket... Perhaps the latter is a better name?

To the extent that you are already touching the documentation of (possibly new) functions, perhaps this is an opportunity to make the documentation a bit more detailed. I find that Haskell documentation (and generally documentation derived from comments in source code rather than written separately as with manpages) is often too terse, and examples are often not provided. It would be good to provide more detailed usage guidance.

[kazu-yamamoto]

Largely, yes, but I see that #423 has touchFdSocket rather than touchSocket... Perhaps the latter is a better name?

Good point!

[kazu-yamamoto]

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