maoe / lifted-async

Run lifted IO operations asynchronously and wait for their results
http://hackage.haskell.org/package/lifted-async
BSD 3-Clause "New" or "Revised" License
29 stars 13 forks source link

Make lifted-async build with monad-control-1.0 #7

Closed UnkindPartition closed 9 years ago

maoe commented 9 years ago

Thanks. I'll merge this and fix the failing test tomorrow since I have no computer at the moment.

maoe commented 9 years ago

I found waitEither_ to be inconsistent with waitEither. I've pushed a test case to illustrate the issue.

There's a fundamental question when using lifted-async with respect to the monadic effects in a forked computation. That is, should the waiting functions discard all the monadic effects (besides IO), or should they restore the effects? I think the only reasonable answer to the question would be the former, discarding all the effects. But the current lifted-async does the latter (and there's a ticket (#2) for this). Why?

The problem is that we can't get a out of StM m a without using restoreM, which always restores the monadic effects as the name suggests. If there was a way to get a result value without restoring its monadic effects, we could fix the issue. I guess basvandijk/monad-control#12 is the feature we need here.

So for now, lifted-async restores the monadic effects. If you use liftBase in waitEither_, it becomes inconsistent with waitEither because the former discards the effects but the latter restores them.

As far as I understand, we can't write waitEither_ in terms of waitEither due to ambiguous types unfortunately.

Should we accept the inconsistency and add a big caution in haddock, or should we stick to monad-control < 1.0 and ask the author of monad-control to get something like basvandijk/monad-control#12 in?

What do you think?

UnkindPartition commented 9 years ago

Ah, that makes perfect sense.

I think any semantics is ok, but please document this subtlety in the haddocks.

The reason why I think it's ok is that every time you're using monad-control with a stack where StM m a does not equal a, you're asking for trouble. I only use lifting to pass configuration parameters (a stack of ReaderTs and similar transformers).

UnkindPartition commented 9 years ago

Also, you can write waitEither_ in terms of waitEither by introducing a proxy (this one does need ScopedTypeVariables):

waitEither_
  :: forall a b m . MonadBaseControl IO m
  => Proxy (a,b)
  -> Async (StM m a)
  -> Async (StM m b)
  -> m ()
waitEither_ _ a b = void (waitEither a b :: m (Either a b))
maoe commented 9 years ago

I just pushed some changes to feature/7-feurbach on top of your commits. The summary of the changes is as follows:

If we could write those instances for Concurrently that would be great but I couldn't come up with any good idea. Could you review the changes?

UnkindPartition commented 9 years ago

Yeah, I hope to find the time to review during the next few days..

UnkindPartition commented 9 years ago

If you have a separate "Safe" module for StM m a ~ a, it seems strange that functions in that module have types with StM m a. I would expect something like

async :: (MonadBaseControl IO m, StM m a ~ a) => m a -> m (Async a)

Such a type would be easier to read, and would actually provide safety in that you wouldn't be able to accidentally spawn an effectful thread and discard its effects.

UnkindPartition commented 9 years ago

Looking at Concurrent now. First question: if the main instances (Applicative, etc.) only support b ~ IO, why is Concurrent even parameterized on b? It'd be easier just to make it IO and remove the b parameter.

UnkindPartition commented 9 years ago

Here's what I mean:

newtype Concurrently m a = Concurrently { runConcurrently :: m a }
instance (Functor m) => Functor (Concurrently m) where
  fmap f (Concurrently a) = Concurrently $ f <$> a
instance (MonadBaseControl IO m) => Applicative (Concurrently m) where
  pure = Concurrently . pure
  Concurrently fs <*> Concurrently as =
    Concurrently $ uncurry ($) <$> concurrently fs as
UnkindPartition commented 9 years ago

Here's a sketch of how a "safe" Concurrently can be implemented using the constraints package: https://gist.github.com/feuerbach/62d5a4f8c25e493c6db1

maoe commented 9 years ago

Thanks!

The safer signature in the Safe module is a good idea. I'll fix that.

As for the b parameter in Concurrently, that's a workaround for avoiding UndecidableInstances. That was introduced in #4 deliberately.

Concurrently using the constraints package is quite interesting. I think I'm going to switch to it.

maoe commented 9 years ago

Just released as v0.3.0. Thanks!