Thanks for the review!
On Wed, Feb 22, 2012 at 11:15 AM, Johan Tibell
The API looks fine except:
HasSocket - I don't think we want to abstract over sockets here, as we don't do so in the rest of the module.
I know it's a bit ugly, but not having it makes it hard to work with unmanaged sockets (e.g. those buried under Handles). If the functions take a managed Socket, you'd have to say something like: setRecvTimeout (MkSocket fd undefined undefined undefined undefined) 120000000 If they take CInt instead, users of the higher-level API would have to say: setRecvTimeout (fdSocket sock) 120000000 Perhaps representing options with ADTs isn't such a bad idea after all. That way, we could have something like: setSocketOption :: SetSockOpt opt => Socket -> opt -> IO () setRawSocketOption :: SetSockOpt opt => CInt -> opt -> IO () Usage might look like: setSocketOption sock $ RecvTimeout 120000000 Type t <- getSocketOption sock
Seconds and Microseconds - These seem a bit misplaced here. I'd say so with Int.
I use Int64 for Microseconds, to avoid truncation when Int is 32 bits. 2^31-1 microseconds is only 35 minutes and 47.483647 seconds. Perhaps I should just use Int and Int64, and be sure to document what units are used. It'd be nice to use a strongly-typed time package like time-units, but: * It involves conversion to and from Integer * It doesn't tell the whole story. Most quantity values in socket options are truncated and even modified for OS-specific reasons.
setHandleTimeout - If we want to keep it at all it should go in Network and not be conditionally exported.
I agree. setHandleTimeouts should simply be a no-op on non-GHC.
timeouts - we need to think about how this interacts with the I/O manager and provide a consistent API across platforms. I suggest we leave them out until we have done so.
The ideal solution would be to not even need these. GHC ought to have proper IO manager support for Windows, or at least use socket timeouts or similar to prevent OS threads from hanging indefinitely on IO operations. setHandleTimeouts is a workaround, and will be deprecated as soon as it is no longer necessary.
I've thought about doing this change before, but I didn't have time to fully explore the design space. I'm happy to accept these patches if someone does that for me. :)
In particular:
* Should we use an algebraic data type to represent options? I don't think so, for the reason pointed out in one of the source code comments in Joey's library.
We can separate getters and setters using typeclasses GetSockOpt and SetSockOpt, and having newtype wrappers for each socket option. See my setSocketOption example above. I would do this, but I have other things I need to work on.
* Are we covering all possible options? Can there be custom options that can no longer be set by the user since we're enumerating a fixed set of options? * Are there any other design trade-offs? Someone needs to read the getsockopt and setsockopt man pages thoroughly and make sure we cover all the cases. * Are we covering all major OSes in the API?
network-socket-options isn't comprehensive. It currently should have all of the SOL_SOCKET and IPPROTO_TCP options present in both Linux and Windows. It compiles on both. I haven't tested on Mac OS X or FreeBSD. I think it'd be better to start with at least some options, and add more as users ask for them. One problem with adding every option we possibly can right now is that none of the options will get documented. If a user asks for a specific option and provides a reason for needing it, we can include this information in the documentation. -Joey