haskell-distributed / network-transport-tcp

TCP Realisation of Network.Transport
http://haskell-distributed.github.com
BSD 3-Clause "New" or "Revised" License
30 stars 27 forks source link

Optional limits on address and data length #52

Closed avieth closed 7 years ago

avieth commented 7 years ago

A transport has two optional limits:

A test case is included.

Some changes in here are also present in #51. Will resolves conflicts once either of them is merged.

dcoutts commented 7 years ago

@avieth actually I think there's a further simplification we can make here. Instead of using a Maybe for the maximum size we can just always supply a max size, but the default can still be maxBound if we want to. That'd simplify the code path in recvWithLengthLimited since there would only be one code path. Similarly, the limits in TCPParams would not be Maybes and the defaults can be 300 and 4GB (or I'd argue we should have something more sane for the latter like 1Mb)

avieth commented 7 years ago

actually I think there's a further simplification we can make here. Instead of using a Maybe for the maximum size we can just always supply a max size, but the default can still be maxBound if we want to. That'd simplify the code path in recvWithLengthLimited since there would only be one code path. Similarly, the limits in TCPParams would not be Maybes and the defaults can be 300 and 4GB (or I'd argue we should have something more sane for the latter like 1Mb)

I'm in favour of this.

facundominguez commented 7 years ago

LGTM @dcoutts, do you want further changes?

avieth commented 7 years ago

I've made the limits non-optional, using maxBound :: Word32 as the defaults.

I've also included a patch to limit the size of Received chunks. This will be useful in tandem with a more sophisticated QDisc: knowing that the maximum number of bytes in any given event is some manageable number (far less than the current 2^32) a QDisc can be more effective at limiting the amount of data that a peer can push into the system. One could argue that this isn't necessary: the existing receive limit is also a bound on the size of a Received event payload. But then, it could be useful to allow relatively large payloads, while demanding more granular events for the benefit of the QDisc.

facundominguez commented 7 years ago

It looks like this PR needs to rebase master in order to have CI passing.

facundominguez commented 7 years ago

Thanks @avieth!