Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IORef based socket #303

Merged
merged 12 commits into from
Feb 6, 2018
Merged

IORef based socket #303

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b1a37e4
IORef for Socket.
kazu-yamamoto Feb 2, 2018
a20d24a
fix for Windows.
kazu-yamamoto Feb 2, 2018
3de7d80
fix for Linux.
kazu-yamamoto Feb 2, 2018
2a5c69a
moving close from Close to Types.
kazu-yamamoto Feb 2, 2018
96d73e3
renaming Close to Shutdown.
kazu-yamamoto Feb 2, 2018
7916e72
Safe closing and GCing Socket.
kazu-yamamoto Feb 2, 2018
610dcb3
fix a typo.
kazu-yamamoto Feb 5, 2018
7e6eea8
invalidating Socket in socketToHandle.
kazu-yamamoto Feb 5, 2018
40801af
pre-calculating the show string of Socket.
kazu-yamamoto Feb 5, 2018
d536bdd
defining invalidSocket.
kazu-yamamoto Feb 5, 2018
a529f96
a proper error message for socketToHandle.
kazu-yamamoto Feb 6, 2018
d555e65
keeping CInt instead of String in Socket.
kazu-yamamoto Feb 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 7 additions & 11 deletions Network/Socket/Types.hsc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ import Data.IORef (IORef, newIORef, readIORef, atomicModifyIORef', mkWeakIORef)
import Data.Ratio
import Foreign.Marshal.Alloc
import GHC.Conc (closeFdWith)
import System.IO.Unsafe (unsafePerformIO)

#if defined(DOMAIN_SOCKET_SUPPORT)
import Foreign.Marshal.Array
Expand All @@ -74,27 +73,24 @@ import Network.Socket.Imports
-----------------------------------------------------------------------------

-- | Basic type for a socket.
newtype Socket = Socket (IORef CInt)
data Socket = Socket (IORef CInt) String
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you decided to go with String rather than ShortByteString or similar. Perhaps slightly more expensive, but easily changed later, without compatibility issues. Are there any module dependency barriers here (circular dependencies or dependencies we want to avoid) to changing this later? Am I wrong about String being a "more expensive" representation to carry along with the socket?

Of course this is unlikely to matter much...

Copy link
Collaborator Author

@kazu-yamamoto kazu-yamamoto Feb 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GHC 7.6 uses bytestring-0.10.0.2 which does not provide ShortByteString unfortunately.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make more sense to carry around an ascii encoded ByteString and pack it whenever it needs to be shown? Utilizing String here means we are introducing a fair amount of indirection and bloat for a Show instance.

Copy link

@vdukhovni vdukhovni Feb 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my instinct also. (Though of course ByteString show is unpack, but that's just detail).
Perhaps the approach should instead be to store the minimal information needed to avoid having to use unsafePerformIO later. To that end, just the lazy byte string form of the fd is enough.
We could even store an originalFd as a Word32 value, whose different type should be enough to discourage misuse (bypassing the IORef).

data Socket = Socket (IORef CInt) Word32
...
mkSocket :: CInt -> IO Socket
mkSocket fd = do
    ref <- newIORef fd
    let s = Socket ref $ fromIntegral fd
    void $ mkWeakIORef ref $ close s
    return s

and then in the show instance use that:

   show (Socket _ wfd) = "<socket: " ++ show wfd ++ ">"

There's probably no need to pre-format the whole string (was my suggestion, sorry). We could keep in mind that Haskell is lazy, and so the (current) string would I think remain an unevaluated thunk until actually used. Not sure whether the thunk is cheaper to keep around than the string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to data Socket = Socket (IORef CInt) CInt.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine. The field is not exported, and the module in which it is visible is small. So the immutable original fd is unlikely to get misused.


instance Show Socket where
show s = "<socket " ++ show (unsafeFdSocket s) ++ ">"
show (Socket _ str) = str

instance Eq Socket where
Socket ref1 == Socket ref2 = ref1 == ref2
Socket ref1 _ == Socket ref2 _ = ref1 == ref2

-- | Getting a file descriptor from a socket.
fdSocket :: Socket -> IO CInt
fdSocket (Socket ref) = readIORef ref

unsafeFdSocket :: Socket -> CInt
unsafeFdSocket = unsafePerformIO . fdSocket
{-# NOINLINE unsafeFdSocket #-}
fdSocket (Socket ref _) = readIORef ref

-- | Creating a socket from a file descriptor.
mkSocket :: CInt -> IO Socket
mkSocket fd = do
let str = "<socket: " ++ show fd ++ ">"
ref <- newIORef fd
let s = Socket ref
let s = Socket ref str
void $ mkWeakIORef ref $ close s
return s

Expand All @@ -103,7 +99,7 @@ invalidateSocket ::
-> (CInt -> IO a)
-> (CInt -> IO a)
-> IO a
invalidateSocket (Socket ref) errorAction normalAction = do
invalidateSocket (Socket ref _) errorAction normalAction = do
oldfd <- atomicModifyIORef' ref $ \cur -> (-1, cur)
if oldfd == -1 then errorAction oldfd else normalAction oldfd

Expand Down