Eq instance is shady

[treeowl]

ghci> a <- async (pure 3)
ghci> a' = fmap (+1) a
ghci> a == a'
True
ghci> wait a
3
ghci> wait a'
4

That doesn't make any sense to me. I think it would be better to remove the Eq and Ord instances and just provide custom comparison operations that explicitly operate only on the thread IDs.

[treeowl]

We could have a SomeAsync with legitimate instances.

The simple way:

data SomeAsync = forall a. SomeAsync (Async a)
instance Ord SomeAsync where
  compare (SomeAsync (Async t1 _)) (SomeAsync (Async t2 _)) = compare t1 t2

Alternatively, we could do this:

-- Any instead of a proper existential so this can unpack
data Async a = Async
  { asyncThreadId :: !ThreadId
  , _resVar :: !(TMVar (Either SomeException Any))
  , _resMod :: (Any -> a) }

instance Functor Async where
  fmap f (Async tid rv g) = Async tid rv (f . g)
  -- Note that <$ replaces the function instead of adding onto it
  x <$ Async tid rv _ = Async tid rv (const x)

data SomeAsync = SomeAsync_ !ThreadId !(TMVar (Either SomeException Any))
instance Ord SomeAsync where
  compare (SomeAsync tid1 _) (SomeAsync tid2 _) = compare tid1 tid2

pattern SomeAsync :: MyAsync a -> SomeAsync
pattern SomeAsync x <- ((\(SomeAsync_ tid mv) -> MyAsync tid mv (const ())) -> x)
  where
    SomeAsync (MyAsync tid tmv _) = SomeAsync_ tid tmv

Another variation on that last is to make SomeAsync a newtype around Async, using the tricks in the some package. That could be good for things like cancelMany.

[simonmar]

Not a big fan of SomeAsync, why not just have sameAsync :: Async a -> Async b -> Bool ?

[treeowl]

We should have that, yes, but I think sometimes people want to be able to do things like use Asyncs for Set or Map keys for whatever reason. SomeAsync, by any implementation, allows that.

[simonmar]

Ok, fair point.

[treeowl]

cancelMany would also be more useful if it took [SomeAsync] instead of [Async a]. Do you have an opinion about which SomeAsync representation to use? I'm partial to the approach of separating the TMVar from the mapping function myself—while that makes the Async heap object one word larger, it reduces the size of the function closure and allows for a more efficient <$>.

[simonmar]

Not to leave this hanging indefinitely - basically I consider this "bug" to be something of a technicality. If we fix it, the cure better not be worse than the disease, and the disease is pretty mild IMO.