Add safe modules

[enolan]

Closes out #169 according to the plan discussed there. This builds on @Yuras' work.

[kazu-yamamoto]

LGTM.

I would like to ask @winterland1989 to check if conflicts against proposals in #201 exist.

[eborden]

Does it make sense to introduce new String variants that immediately have a warning? If we choose to introduce these then they most definitely must have a warning. The alternative is to not provide them in the first place.

I assume the argument for providing them is that a new-comer might be confused and frustrated when they don't find them. The converse of this is that the existing String functions are already well documented as being obsolete and introducing new String functions could cause more confusion and a maintenance burden.

I'd rather not continue to contribute to the "String problem", so I'd vote against these variants.

[eborden]

Some minor quibbles, but this is looking pretty solid.

[eborden]

Oh also, I'd like to see haddock documentation put in place for all of these new modules. Nothing fancy, likely the previous documentation can be copied over with minor tweaks.

Thanks!

[winterland1989]

I'm in favor of document improvement but minimal API change now, add a simple withOpenedSocket :: (Socket -> IO a) -> IO (Either SocketStatus a) to lock the Mvar is good enough for me, the main reasons are:

• network provide low-level operations, which is good because it keep the same API with unix's abstraction with little overhead.
• Adding a new module will add maintenance overhead forever for core library like this.
• A program should not be designed to rely on exception like this, should we?

My head went 180 degree since i worked with network for a month, at first i though we should provide more high level operations so that users can do things right at first, but now i realize the unix API itself is full of choices, and i think we should provide build block instead of make choices for user.

[eborden]

@winterland1989 You raise some great points. The utility of a high order function is certainly greater than introducing safe modules that must be adjusted every time an api changes, etc.

The benefit that Safe modules provide is discover-ability. withOpenedSocket has a much higher level of utility, but it doesn't have a big bright label on it like a Safe module.

So the question then becomes weighing the benefits and costs:

modules

pro:

• easily discoverable
• blessed path
• duplicate interfaces that are familiar

con:

• introduces a persistent maintenance burden, apis and documentation must be kept in sync.

function

pro:

• it's a high order function with a simple api
• It is compose-able.
• has a low surface area, thus less maintenance.

con:

• less discoverable
• (Socket -> IO) is slightly awkward with the existing api, forcing the use of lambdas or flip.

The middle path is introducing the Safe module that only exposes withOpenedSocket and becomes a home for other "safe" functions, but that might be too heavy a solution.

bike-shed: withOpenSocket?

[winterland1989]

The middle path is introducing the Safe module that only exposes withOpenedSocket and becomes a home for other "safe" functions, but that might be too heavy a solution.

Yeah, I don't think another module is really necessary, but let's wait @kazu-yamamoto 's judgement on this.

In fact network's existing module need more care: cleaning CPPs, adding document etc.

[kazu-yamamoto]

I think withOpenSocket should be provided in the network package.

Safe modules are definitely necessary. But question is that Safe modules should be provided in the network package or in another package, say, network-safe? Any opinions?

[winterland1989]

AFAICT, safe operations are only needed whenSocket is used by other threads, where Socket should be protected by a MVar. In one Socket per thread fashion, you shouldn't need safe operations unless you're doing something badly wrong, is my understanding right?

withOpenSocket leverage MVar field in Socket to lock the Socket with addition status check, so it's reasonable to be provided by network.

[kazu-yamamoto]

@winterland1989 I think you misunderstand the motivation.

Please read #169.

[winterland1989]

Sorry for missing the context, but from the linked issue, the problem is that warp's reaper/http-client's timeout are closing Socket from other thread. If user use safe operations, then user are forced to handle exceptions, which is ironic because safe usually means no exception.

Bottom line: I'm still in favor of documenting about protecting Socket in multithread situations rather than throwing exceptions when people use safe operations.

[winterland1989]

To make my point clear, i propose:

-- | If you're operating `Socket` in multithread environment,
-- for example, use a timeout thread to close `Socket`. you have to
-- make sure `Socket` is opened before read/write, 'withConnectedSocket'
-- will run your action if `Socket` is still 'Connected', otherwise
-- return the 'Socket' status instead.
--
-- Note, this will block other thread which try to close `Socket` by locking
-- the status `MVar`.
withConnectedSocket :: Socket -> (Socket -> IO a) -> IO (Either SocketStatus a)
withConnectedSocket sock@(MkSocket _ _ _ _ statusVar) act =
  withMVar statusVar $ \status ->
     case status of
       Connected -> act sock
       _         -> return (Left status)

I prefer dealing with Either rather than catching UseError ; )

[eborden]

@winterland1989 I agree on your point of utilizing Either instead of exceptions.

@kazu-yamamoto Creating network-safe sounds like the right tact instead of adding Safe modules to network proper.

[eborden]

We could also further abstract to the substrate:

withSockStatus :: Socket -> (Socket -> SocketStatus -> IO a) -> IO a
withSockStatus s f = withMVar (sockStatus s) $ f s

We can then build from there with connected, bound, listening, etc.

[enolan]

Two initial points:

• I have to disagree about using an Either rather than throwing an exception. Using Either to signal errors and using exceptions are both valid choices, but throwing exceptions for most and using Either for one is silly. The network API is already full of exceptions.
• The problem we're trying to prevent is analogous to use-after-free. Multithreading only makes it easier to write the bug.

What's needed to close #169 and make a release? My purpose in doing this was to get a version of network with #192 up on Hackage so I can finish the work that originally motivated fixing that bug.

I can either:

• Fix up this PR to have better associated haddock documentation. If the maintainers think it's best I'll copy paste the documentation from the underlying functions. However, I think it's fine as it is now. For reference, this is what it looks like. When I imagine someone using Network.Socket.Safe, they aren't looking at Network.Socket.Safe's Haddock, they're looking at Network.Socket's. The APIs are exactly the same, the only difference is checking that the given socket isn't closed before using it. Or:
• Remove the Safe modules and export some version of wrapCheckStatus or withConnectedSocket. I'm not going to make a new package for the safe modules, but my code is of course BSD licensed and anyone can if they wish.

[kazu-yamamoto]

I think that we merged necessary documentation updates. Let's release the new version first.
And let's continue the safe stuff.

[winterland1989]

This document made an interesting point: Socket is supposed to be a duplex channel, some applications are suppose to recv/send on different threads simultaneously, we should not hold locks during recv/send(although i have never designed such a system).

It seems socket package workaround this with a complex loop. I'm not clear if it's worth to do.

[kazu-yamamoto]

I close this in favor of #212.