Don't allowInterrupt inside uninterruptibleMask
AbandonedPublic

Authored by bgamari on Aug 28 2014, 12:23 PM.

Details

Summary

This fixes Trac #9516.

Test Plan

Review. In particular, we need to decide if we want to export interruptible from Control.Exception or not.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
fix-allowInterrupt
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 660
Build 662: GHC Patch Validation (amd64/Linux)
edsko updated this revision to Diff 453.Aug 28 2014, 12:23 PM
edsko retitled this revision from to Don't allowInterrupt inside uninterruptibleMask.
edsko updated this object.
edsko edited the test plan for this revision. (Show Details)
edsko added reviewers: austin, simonmar.
edsko updated the Trac tickets for this revision.
austin added subscribers: ekmett, hvr.

LGTM. But:

  1. We should add a test. I imagine with a trivial example it should be easy to verify that allowInterrupt (the real bug) works properly now, no?
  2. Regarding bikeshedding on the exports: we already do export other mask-related facilities from Control.Exception, so adding another 'safe' export (vs unsafeMask) seems reasonable to me. Some other people should weigh in.

I'm adding @hvr and @ekmett to see if they have any input, re: point 2.

austin requested changes to this revision.Aug 28 2014, 12:37 PM
austin edited edge metadata.
This revision now requires changes to proceed.Aug 28 2014, 12:37 PM
edsko updated this revision to Diff 454.Aug 28 2014, 12:46 PM
edsko edited edge metadata.

Don't import unsafeUnmask.

ekmett accepted this revision.Aug 28 2014, 1:19 PM
ekmett edited edge metadata.

I have no objection to exporting interruptible.

It looks good to me.

austin requested changes to this revision.Aug 28 2014, 1:52 PM
austin edited edge metadata.
austin added a subscriber: simonpj.

The test failures are benign, probably caused by @simonpj earlier I assume. It would still be good to have a test for this bug, though.

This revision now requires changes to proceed.Aug 28 2014, 1:52 PM
simonmar edited edge metadata.EditedAug 28 2014, 3:35 PM

Actually I'm not sure about interruptible, because it sidesteps the scoping we get with mask. The restore function passed to mask can only unmask exceptions if the enclosing context was unmasked, whereas interruptible as defined here unconditionally unmasks interrupts. Therefore it is unsafe, like unsafeUnmask but less so (it only punches a hole in mask, not uninterruptibleMask). So if we have it, it should have "unsafe" in its name.

edsko added a comment.Aug 28 2014, 4:00 PM

I don't insist on the name, of course. However, it seems to me that "punching a hole through mask" is precisely what "interruptible" means. When we say that takeMVar is interruptible, that is what we mean, I think?

(I came across the problem in the first place precisely because I wanted to be able to define my own interruptible resource allocation functions; see http://www.well-typed.com/blog/97/ ).

Then again, I suppose it can be argued even takeMVar is dangerous and should really be called unsafeTakeMVar for the same reason :)

I don't mind either way.

Ok, I agree. Let's call it interruptible.

ezyang removed a subscriber: ezyang.Oct 27 2014, 2:05 PM
Yuras added a subscriber: Yuras.Nov 15 2014, 12:01 PM
bgamari requested changes to this revision.Jul 27 2015, 7:49 AM
bgamari added a reviewer: bgamari.
bgamari added a subscriber: bgamari.

It sounds like this is nearly ready to be merged. There are a few issues, however.

libraries/base/Control/Exception.hs
218

While we are at it, this markup is incorrect: it defines an anchor named "interruptible" where it should be referring to interruptible (which I believe should look like 'interruptible').

libraries/base/GHC/IO.hs
338

Is there any reason for this "(see #interruptible#)" note here? Aren't we currently seeing interruptible?

bgamari commandeered this revision.Jul 27 2015, 8:05 AM
bgamari edited reviewers, added: edsko; removed: bgamari.
bgamari added inline comments.
libraries/base/GHC/IO.hs
338

Oops, never mind. I see.

bgamari abandoned this revision.Aug 3 2015, 1:59 AM

Merged as 5a8a8a64e793d2efbe9ea7d445cc8efe75d11f80.