Skip to content

Make Async a Hashable #73

Open
Open

Description

@kirelagin

Async a already has instances of Eq and Ord, which makes it possible to store it in a Set, but it has no instance of Hashable, so it is not possible to store it in a HashSet, while the idea of having a HashSet of running threads seems to be quite useful.

Given that ThreadId already has an instance of Hashable and Eq and Ord of Async a delegate to the internal ThreadId, providing such an instance would be trivial, although it would require depending on hashable.

If this is acceptable, I can prepare a PR.

Activity

  1. changed the title [-]Make Async Hashable[/-] [+]Make `Async a` `Hashable`[/+] on Dec 19, 2017
  2. simonmar commented on Dec 21, 2017

    @simonmar
    Owner

    Yes!

  3. sol commented on Jan 6, 2018

    @sol
    Contributor

    I'm not too excited about this change for

    • the added dependency on hashable (which e.g. currently does not build with GHC HEAD)
    • the transitive dependency on text, which is (a) heavy weight (b) not a boot library with older versions of GHC

    While I understand the virtue of this change, it also induces a cost on every user of async.

    How I see it, this is a trade of between cost and functionality. Making a decision either way is valid. I just wanted to voice my preference here, which is not adding the additional dependencies and keeping async as lightweight as possible.

  4. kirelagin commented on Jan 6, 2018

    @kirelagin
    ContributorAuthor

    Yeah, I do realise that it is somewhat of a compromise, that’s why I started by asking whether Simon considers adding a dependency on hashable possible.

    And I agree that some users may see this as an additional cost they did not ask for. But, on the other hand, I believe that any non-trivial Haskell project is very likely to depend on text and probably on hashable too.

    In the end it should be your decision. Another option (rather weird one, but probably easier on the users) would be to have a separate library (e.g. async-hashable) for this sole purpose.

  5. simonmar commented on Jan 8, 2018

    @simonmar
    Owner

    The alternative is to have hashable depend on async. I don't feel strongly either way, but if hashable already has a large set of dependencies perhaps this is the better way to go. Putting the instance in a separate library is not good because orphans.

  6. kirelagin commented on Jan 8, 2018

    @kirelagin
    ContributorAuthor

    if hashable already has a large set of dependencies

    It does not :(

      Build-depends:     base >= 4.4 && < 4.11,
                         bytestring >= 0.9 && < 0.11,
                         deepseq >= 1.3
      if impl(ghc)
        Build-depends:   ghc-prim,
                         text >= 0.11.0.5
      if impl(ghc) && flag(integer-gmp)
        Build-depends:   integer-gmp >= 0.2

    We can ask what they think nevertheless, but I suspect they won’t be too excited about a transitive dependency on stm either.

  7. LeifW commented on Jan 8, 2018

    @LeifW
  8. simonmar commented on Jan 8, 2018

    @simonmar
    Owner

    You could add it as an optionally built module + dep via cabal flag?

    No, that's even worse. Packages should have a stable API. How would you express a dependency on the version of async with the flag enabled?

    FWIW, GHC will have a dependency on text soon, so I don't think a dependency on text is terrible.

  9. sol commented on Feb 10, 2018

    @sol
    Contributor

    For hspec/hspec I now include a "vendored" copy of async without the hashable dependency. It's ok with me to close this issue.

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions