Opened 7 years ago

Closed 7 years ago

#7417 closed bug (fixed)

replace Control.Concurrent.QSem

Reported by: tibbe Owned by:
Priority: normal Milestone: 7.8.1
Component: libraries/base Version: 7.6.1
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

Right now there's no replacement for packages in the HP (e.g. Cabal), leading to all kinds of ugly workarounds. See https://github.com/haskell/cabal/pull/1108 for some of the pain this causes.

If QSem is broken we should fix it, not deprecate it and let users deal with the difficulty of finding a correct semaphore implementation that they can use.

Change History (5)

comment:1 Changed 7 years ago by igloo

difficulty: Unknown
Resolution: wontfix
Status: newclosed

There's no need to have semaphore code in base, nor any package that comes with GHC.

If the Haskell Platform wants to provide semaphore functionality then it can add a package that provides it, either by adding SafeSemaphore, by adding a new package containing a fixed version of the code that used to be in base, or adding some other package.

comment:2 Changed 7 years ago by duncan

Resolution: wontfix
Status: closednew

I don't think the fact that it's broken is the most important thing here. When we move module out of base (especially ones that have been around for ages and ages) we do have a duty to provide a new solution and make the transition smooth. We have done that in the past when moving things out of base. The fact that the current code is buggy in some cases doesn't remove that obligation.

In addition, I think we could have a much longer deprecation period here before removal. The code clearly works for the vast majority of cases (or we'd have found the bug years ago). In fact ticket #3160 makes clear that it's only broken for cases where you kill threads with async exceptions.

comment:3 Changed 7 years ago by igloo

Resolution: wontfix
Status: newclosed

We've already agreed (unanimously, I think) to remove them from base (http://www.haskell.org/pipermail/libraries/2012-June/thread.html#17881) and they have already been removed in HEAD. Nothing else in base, or the rest of a GHC tree, depends on it, so there is no need for it to be in base. Additionally, we, as base maintainers, have not been maintaining this code (as evidenced by #3160 not being fixed for years), and having it in base means that it will take months (if not years) for any bug fixes that people do make to reach end users.

If you, or anyone else, wants the old implementation then please feel free to put the old code in a standalone package on Hackage, which could be added to the HP if the HP also wants it. At this point, this would scarcely be any more effort than re-adding it to base. If no-one is willing to do this before GHC 7.8.1 is released (that's at least 2 months away, and probably more) then it will surely never be done.

comment:4 Changed 7 years ago by simonmar

Resolution: wontfix
Status: closednew
Summary: Undeprecate Control.Concurrent.QSemreplace Control.Concurrent.QSem

Ian is right, there's no good reason to have these in base, except that we inconvenience users by removing them without making alternative provisions (Duncan's point). So I propose that we

  • make sure there's a clear replacement
  • arrange that it will be in the platform when the platform adopts 7.8

and if we can't do these, then we should put the original modules back. (I think we could do without mergeIO, but at least QSem is important).

I've been experimenting with semaphore implementations: https://github.com/simonmar/sem

The one in https://github.com/simonmar/sem/blob/master/Sem2.hs is reasonably simple, safe from async exceptions, and has good performance (better than the ones in SafeSemaphore). We could make a new package, with this as a starting point perhaps? It's only a tiny bit of code though.

comment:5 Changed 7 years ago by simonmar

Milestone: 7.8.1
Resolution: fixed
Status: newclosed

I added my new QSem and QSemN implementations as an interrim measure to avoid breaking code:

commit ea3abf16eda97e573ee63fb08ce330d3aeceaeae
Author: Simon Marlow <marlowsd@gmail.com>
Date:   Mon Dec 10 13:39:11 2012 +0000

    Add back new working QSem and QSemN implementations (#7417)
    
    We decided not to break existing users without providing an easy
    migration path.  For the time being I've made these implementations,
    which fix the bugs in the old versions and perform reasonably well.
    
    In due course we should move the concurrency functionality, including
    these modules, out of base and into a separate package.
Note: See TracTickets for help on using tickets.