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

Remove invalid instance Monad Concurrently. #26

Merged
merged 1 commit into from Apr 26, 2015

Conversation

Projects
None yet
8 participants
@ygale

ygale commented Apr 23, 2015

The Monad instance for Concurrently is invalid: we have (<*>) /= ap because ap runs the two actions sequentially while (<*>) runs them concurrently. There is no way to write a valid Monad instance for Concurrently, so the existing broken instance should just be removed.

The existence of an invalid Monad instance causes breakage in the wild. See, for example, ekmett/either#38.

@ygale

This comment has been minimized.

ygale commented Apr 23, 2015

Summary of discussion so far, which started in ekmett/either#38:

Based on an idea of @glguy, I thought that there is no way to implement a Monad instance consistent with the Applicative one, because to implement monadic bind, you need to complete the action on the LHS before applying the function on the RHS to its result. So I created the original version of this PR which completely removes the Monad instance.

However, now I realize that in fact it is possible to create a concurrent Monad instance, by exploiting laziness via mdo. However, that also uses some MVar magic internally. So the resulting ap would have operational semantics that are very similar to (<*>), but still not necessarily exactly the same. Should we do that, or just dump the Monad instance?

@evincarofautumn

This comment has been minimized.

evincarofautumn commented Apr 23, 2015

Furthermore, when ApplicativeDo is implemented, the Monad instance should be unnecessary—presumably you would want compilation to fail if you try to do something Concurrently that can’t be.

@glguy

This comment has been minimized.

glguy commented Apr 23, 2015

For reference this instance was added due to #17 and #18

@ygale

This comment has been minimized.

ygale commented Apr 23, 2015

Forget about mdo. Using only unsafeInterleaveIO as suggested by @evincarofautumn, here is shot at it:

mx >>= f = withAsync ma $ \ax -> unsafeInterleaveIO (wait ax) >>= f

I am a little worried that the laziness can result in some unexpected behavior beyond the concurrency though.

@nikita-volkov

This comment has been minimized.

nikita-volkov commented Apr 23, 2015

You cannot have a Monad instance, which executes things concurrently, because due to the nature of Monad a further action depends on the result of a preceding one. Here's a simple example:

do
  decision <- decide
  doSomethingBasedOnADecision decision

Executing doSomethingBasedOnADecision concurrently with decide simply doesn't make sense.

However, I absolutely disagree with the following statement:

invalid: we have (<*>) /= ap

I explain my position in the parallel thread.

@rwbarton

This comment has been minimized.

rwbarton commented Apr 23, 2015

Why does it simply not make sense? doSomethingBasedOnADecision might do some things before needing to consult the value of decision, and those can be executed concurrently with decide. In the extreme case, such as when implementing ap, the f in m >>= f could perform all of its IO effects without ever forcing the result produced by m.

@ekmett

This comment has been minimized.

ekmett commented Apr 24, 2015

Personally, I'd favor just removing it, rather than trying to come up with a super-fancy version that buries unsafePerformIO'd MVar reads in normal variables.

@ygale

This comment has been minimized.

ygale commented Apr 25, 2015

@ekmett It's just unsafeInterleaveIO, which is what this whole library is doing anyway. But I too am hesitant.

I don't feel confident enough of my understanding of this library to be certain that throwing laziness into the mix in this way doesn't create some subtle difference in semantics that we don't want.

And is my proposed implementation good enough, or do we need another withAsync? Like this:

(Concurrently mx) >>= f = withAsync mx $ \ax -> do
    x <- unsafeInterleaveIO $ wait ax
    withAsync (f x) wait

The intended interface for Concurrently is the Applicative one. So I am taking the easy way out for now and leaving the PR as is, cowardly dumping the Monad instance altogether.

@ygale

This comment has been minimized.

ygale commented Apr 25, 2015

Fixing the typos in my two proposals for a Monad instance. The originals don't type check.

(Concurrently mx) >>= f = Concurrently . withAsync mx $ \ax -> do
    x <- unsafeInterleaveIO $ wait ax
    runConcurrently $ f x

(Concurrently mx) >>= f = Concurrently . withAsync mx $ ax -> do
    x <- unsafeInterleaveIO $ wait ax
    withAsync (runConcurrently $ f x) wait
@simonmar

This comment has been minimized.

Owner

simonmar commented Apr 26, 2015

Even if it were palatable, which it is not, I believe the unsafeInterleaveIO version is still incorrect, because exceptions. Exceptions are part of the semantics of IO, so we can't ignore them. The unsafeInterleaveIO version makes ap = <*> non-deterministically, which isn't enough.

simonmar added a commit that referenced this pull request Apr 26, 2015

Merge pull request #26 from ygale/NoMonadConcurrently
Remove invalid instance Monad Concurrently.

@simonmar simonmar merged commit bb1d405 into simonmar:master Apr 26, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@simonmar

This comment has been minimized.

Owner

simonmar commented Apr 26, 2015

Ah, I see now - the idea is to make a Monad instance that makes ap behave like the existing <*>, which is already non-deterministic. Ok. But I agree with Ed, it's pretty unsavoury to have a >>= based on unsafeInterleaveIO. The rest of the API makes waiting for an Async an IO operation, rather than leaking it into the pure world. Do we want exceptions from Asyncs being thrown by apparently pure computations? I suspect not.

@ygale

This comment has been minimized.

ygale commented Apr 26, 2015

@simonmar Agreed. Thanks for reviewing this!

@ygale ygale deleted the ygale:NoMonadConcurrently branch Apr 26, 2015

@tomberek

This comment has been minimized.

tomberek commented Aug 25, 2015

@simonmar @ygale: I ran into this issue while trying to apply Kleisli to Concurrently. Eg.

getHTTPResponseCode :: URL -> IO HTTPCode
i would like to have
getHTTPResponseCode :: URL -> Concurently HTTPCode
let myArrow = Kleisli getHTTPResponseode :: Kleisli Concurrently URL HTTPCode
let getPair = myArrow1 *** myArrow2 :: Kleisli Concurrently (URL,URL) (HTTPCode,HTTPCode)
-- or some much more complicated arrow expression ^
let getPair2 = runKleisli getPair :: (URL,URL) -> Concurrently (HTTPCode,HTTPCode)
let getPair3 = runConcurrently $ getPair2 (foo,bar) :: IO (HTTPCode,HTTPCode) 
-- the requests are made in parallel.

Applicative is not powerful enough to allow a Applicative f => a -> f a version of Kleisli. A monad instance of Concurrently would allow this navigation through arrows.

@ekmett

This comment has been minimized.

ekmett commented Aug 25, 2015

@tomberek The point of the above discussion is that Concurrently's Monad and its Applicative offer incompatible behavior.

Losing the Monad means you of course can't use Kleisli to make an arrow -- its not a Monad any more.

But that isn't the only arrow you could have. Notably the static arrow transformer

newtype Static f p a b = Static { runStatic :: f (p a b) }

takes any Applicative f and transforms some other arrow p with it.

so if we start myArrow = Kleisli getHTTPResponse :: Kleisli IO URL HTTPCode

you should be able to do something like lift it into Static Concurrently (Kleisli IO) URL HTTPCode and proceed more or less as above. Now the (***) of the arrow is responsible for sticking together parts using Concurrently and lift should embed simpler Kleisli IO arrows into this arrow.

@tomberek

This comment has been minimized.

tomberek commented Aug 26, 2015

Static Concurrently (Kleisli IO) URL HTTPCode requires a value of type Concurrently (Kleisli IO URL HTTPCode). That contains a value of type: IO (Kleisli IO URL HTTPCode). So what we've really gained is the ability to create the computation concurrently, not run the computation concurrently. This may be valuable, but not the original intent. After some arrow composition, one would need a way to convert:

Concurrently (Kleisli IO (URL,URL) (HTTPCode,HTTPCode) -> (URL,URL) -> Concurrently (HTTPCode,HTTPCode)
or to get two separate Concurrently's out of it which we then run with (a,b) <- runConcurrently $ (,) <$> c1 <*> c2

The expected runKleisli <$> myArrow <*> (myurl1,myurl2) doesn't help because it results in a `Concurrently (IO (HTTPCode,HTTPCode)). runConcurrently is only creating the computation, not running the HTTP requests.

I've been able to make the behavior work right with defining a newtype PKleisli a b = PKleisli {runPKleisli:: Kleisli IO a b} and then defining (***) with concurrently, I was only hoping to be get some fancy re-use withConcurrently`. Or am I missing something obvious?

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