Fix the `fromSocket` limit to be 4096

[k0001]

This is probably a good idea.

[kvanbere]

Actually there's some good reasons to use a much smaller size or even roll your own malloc based receiver.

See "Buffer allocation to receive HTTP requests" here: http://www.yesodweb.com/blog/2014/02/new-warp .

[PierreR]

Here is an suggestion using the record syntax to implement optional/default/named params :

opts = Options 4096 Nothing

data Options = Options
                { _nbytes :: Int
                , _wait :: Maybe Int
                }

fromSocket
  :: MonadIO m
  => Socket     -- ^Connected socket.
  -> Options
  -> Producer' B.ByteString m ()
fromSocket sock Options {_nbytes=nbytes, _wait=Nothing}  = loop where
    ....

fromSocket sock Options {_nbytes=nbytes, _wait=Just(wait)}  = loop where
    loop = do
     ...
{-# INLINABLE fromSocket #-}

And it would be used like this:

       fromSocket client opts
-- or
       fromSocket client opts{_nbytes=128}
       fromSocket client opts{_nbytes=128, _wait=Just 120}

I am not convinced and came to the unfortunate conclusion that Haskell is clumsy here.

Anyhow if there is a consensus for this kind of stuff (which I believe is unlikely), I would be happy to submit a PR (of course it would take care of toSocket as well)

Let me know if there is a way to do better.

[k0001]

@PierreR I like your proposal. In particular, I like that we can remove the “timeout” variants of these functions. Please go ahead with these changes if you want to do so.

I'm not sure what to do in fromSocketN/fromSocketTimeoutN though. Maybe it should just take the same Options as argument and ignore the value of the _nbytes field? Hmm, or maybe the the downstream Proxy can request using an Option so that the type of fromSocketN looks like this:

fromSocketN 
  :: MonadIO m 
  => Socket -> Options -> Server' Options B.ByteString m ()

That's fine by me. We should rename fromSocketN to something better, though.

[k0001]

Hmmm… actually, I'm not sure if this is a good idea. I guess toSocket won't care about the _nbytes field either, which means we'll either have to pass the suggested Options and have toSocket ignore the _nbytes field, or define another SendOptions type (which I don't like, as it hinders the simplicity of the API).

Do you have any suggestions about this?

[PierreR]

I will have to check about this but using a type class might do the trick.

This is what data-default is doing and is actually suggested by both Neil Mitchell and Brent Yorgey.

But ignoring _nbytes would work fine:

toSocket
  :: MonadIO m
  => Socket  -- ^Connected socket.
  -> Options
  -> Consumer' B.ByteString m r
toSocket sock Options {_nbytes=_, _wait=Nothing} = for cat (\a -> send sock a)

toSocket sock Options {_nbytes=_, _wait=Just(wait)} = for cat $ \a -> do
    mu <- liftIO (timeout wait (NSB.sendAll sock a))
    case mu of
       Just () -> return ()
       Nothing -> liftIO $ ioError $ errnoToIOError
          "Pipes.Network.TCP.toSocketTimeout" eTIMEDOUT Nothing Nothing
{-# INLINABLE toSocket #-}

[kvanbere]

Make sure if you do so, that you guard timeout so it doesn't get called if the user does not specify one. Personally, I would prefer that the functions are kept distinctly separate.

[PierreR]

I don't think there is a risk as the default for wait is Nothing. That said I understand your preference ;-) I actually don't mind the 2 separate functions, what was bothering me more is the lack of a sane default for nbytes. There might be a better solution.

[k0001]

Any positive integer is a “safe default”, but the ideal value depends on the software you are building and the network on which it'll run. I'm actually quite happy about the current situation, I'm not sure it needs “fixing” at all.

That said: I don't really like the idea of adding a dependency on data-default as mentioned above just for this.

[Gabriella439]

I also don't like data-default. It's basically the epitome of an unprincipled type class: it obeys no laws. The other problem is that it implies that there is only one sensible default, but what would happen if we wanted to provide two alternative defaults, i.e. one optimized for maximizing bandwidth and another one optimized for minimizing memory overhead?

I think that if you do the config route just provide the default configuration as an explicit record and don't use any type class for it.

[PierreR]

Ah, that's good to know. Anyhow I don't see how it is possible to pattern match on a type class ... So the type class route looks like a dead end.

Isn't 4096 a decent default for most cases ? If this is untrue, I agree there is no need for a fix.

Anyhow I will leave it for now. The record configuration is not that convincing (mainly because of the introduction of opts).

[jhenahan]

Would a minor indirection be acceptable?

fromSocket sock = fromSocket' sock 4096

with fromSocket' being defined as fromSocket currently is, maybe? Perhaps the other way around if the more common use case is specifying nbytes.

EDIT: Alternatively, we could go all out with the record idea and

data PSocket = PSocket { sock :: Socket, nbytes :: Int, wait :: Int }
defaults s = PSocket  { sock = s, nbytes = 4096, wait = 200 }

or something like that.

[k0001]

I'll close this as the alternatives to providing the number of bytes to receive seem to be even more cumbersome. I'm personally quite happy with the current approach, and since the discussion seem to have stopped some months ago, I'll close this issue now. However, if someone still quite dislikes it and want to propose something different, please reopen this issue.