New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Dec 28, 2014

Conversation

2 participants
@feuerbach
Contributor

feuerbach commented Dec 16, 2014

No description provided.

@maoe

This comment has been minimized.

Show comment
Hide comment
@maoe

maoe Dec 16, 2014

Owner

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

Owner

maoe commented Dec 16, 2014

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

@@ -2,6 +2,7 @@
{-# LANGUAGE KindSignatures #-}
{-# LANGUAGE RankNTypes #-}
{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE ScopedTypeVariables #-}

This comment has been minimized.

@maoe

maoe Dec 16, 2014

Owner

I don't think this extension is necessary.

@maoe

maoe Dec 16, 2014

Owner

I don't think this extension is necessary.

@maoe

This comment has been minimized.

Show comment
Hide comment
@maoe

maoe Dec 17, 2014

Owner

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?

Owner

maoe commented Dec 17, 2014

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?

@feuerbach

This comment has been minimized.

Show comment
Hide comment
@feuerbach

feuerbach Dec 17, 2014

Contributor

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).

Contributor

feuerbach commented Dec 17, 2014

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).

@feuerbach

This comment has been minimized.

Show comment
Hide comment
@feuerbach

feuerbach Dec 17, 2014

Contributor

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))
Contributor

feuerbach commented Dec 17, 2014

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))

feuerbach referenced this pull request in snoyberg/lifted-async Dec 22, 2014

Fix some monad-control 1.0 breakage
Unfortunately doesn't fix everything, and I'm stumped on the last bit,
namely:

[1 of 1] Compiling Control.Concurrent.Async.Lifted (
src/Control/Concurrent/Async/Lifted.hs,
dist/build/Control/Concurrent/Async/Lifted.o )

src/Control/Concurrent/Async/Lifted.hs:263:6:
    Could not deduce (StM m b0 ~ StM m b)
    from the context (MonadBaseControl IO m)
      bound by the type signature for
                 waitEither_ :: MonadBaseControl IO m =>
                                Async (StM m a) -> Async (StM m b) -> m
()
      at src/Control/Concurrent/Async/Lifted.hs:(263,6)-(266,9)
    NB: ‘StM’ is a type function, and may not be injective
    The type variable ‘b0’ is ambiguous
    Expected type: Async (StM m a) -> Async (StM m b) -> m ()
      Actual type: Async (StM m a0) -> Async (StM m b0) -> m ()
    In the ambiguity check for:
      forall (m :: * -> *) a b.
      MonadBaseControl IO m =>
      Async (StM m a) -> Async (StM m b) -> m ()
    To defer the ambiguity check to use sites, enable
AllowAmbiguousTypes
    In the type signature for ‘waitEither_’:
      waitEither_ :: MonadBaseControl IO m =>
                     Async (StM m a) -> Async (StM m b) -> m ()
@maoe

This comment has been minimized.

Show comment
Hide comment
@maoe

maoe Dec 22, 2014

Owner

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

  • Fix the failing test case for waitEither_.
  • Add a few haddock comments about the behavior of monadic effects.
  • Add Control.Concurrent.Async.Lifted.Safe module which is only applicable when StM m a ~ a.
    • Caveat: With this constraints, we can't write Applicative/Monad instances for Concurrently.

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?

Owner

maoe commented Dec 22, 2014

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

  • Fix the failing test case for waitEither_.
  • Add a few haddock comments about the behavior of monadic effects.
  • Add Control.Concurrent.Async.Lifted.Safe module which is only applicable when StM m a ~ a.
    • Caveat: With this constraints, we can't write Applicative/Monad instances for Concurrently.

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?

@feuerbach

This comment has been minimized.

Show comment
Hide comment
@feuerbach

feuerbach Dec 22, 2014

Contributor

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

Contributor

feuerbach commented Dec 22, 2014

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

@feuerbach

This comment has been minimized.

Show comment
Hide comment
@feuerbach

feuerbach Dec 25, 2014

Contributor

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.

Contributor

feuerbach commented Dec 25, 2014

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.

@feuerbach

This comment has been minimized.

Show comment
Hide comment
@feuerbach

feuerbach Dec 25, 2014

Contributor

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.

Contributor

feuerbach commented Dec 25, 2014

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.

@feuerbach

This comment has been minimized.

Show comment
Hide comment
@feuerbach

feuerbach Dec 25, 2014

Contributor

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
Contributor

feuerbach commented Dec 25, 2014

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
@feuerbach

This comment has been minimized.

Show comment
Hide comment
@feuerbach

feuerbach Dec 25, 2014

Contributor

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

Contributor

feuerbach commented Dec 25, 2014

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

@maoe

This comment has been minimized.

Show comment
Hide comment
@maoe

maoe Dec 25, 2014

Owner

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.

Owner

maoe commented Dec 25, 2014

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 maoe merged commit 764fc2e into maoe:develop Dec 28, 2014

1 check failed

continuous-integration/travis-ci The Travis CI build failed
Details
@maoe

This comment has been minimized.

Show comment
Hide comment
@maoe

maoe Dec 28, 2014

Owner

Just released as v0.3.0. Thanks!

Owner

maoe commented Dec 28, 2014

Just released as v0.3.0. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment