[m-rev.] for review: Delete closeable_channel.is_closed.

Peter Wang novalazy at gmail.com
Fri Apr 17 17:04:05 AEST 2020


This is a plausible sequence of events leading to a failure in the
hard_coded/closeable_channel_test.m test case:

     thread A                   thread B
     --------                   --------
     close(Chan)
                                channel.take(Chan)
                                   -> mvar.read(Head)
                                        -> semaphore.wait(Full)
     is_closed(Chan, IsClosed)
          -> mvar.try_read(Hole)
               -> semaphore.try_wait(Full)

Thread B tries to take the next item from the channel, during which time
it holds the Full semaphore of the from the mvar at the read end of the
channel.

Thread A attempts to verify that the channel is really closed.
It tries to read from the mvar at the write end of the channel, which is
the same mvar as thread B is reading from (because the channel is empty).
Thread A cannot acquire the Full semaphore of the mvar as it is being
held by thread B, resulting in the call is_closed(Chan, IsClosed) to
return with IsClosed = no.

library/thread.closeable_channel.m:
    Delete the is_closed predicate. It was of questionable utility
    anyway.

tests/hard_coded/closeable_channel_test.m:
    Delete call to is_closed in test case.
---
 library/thread.closeable_channel.m        | 32 +++--------------------
 tests/hard_coded/closeable_channel_test.m |  9 -------
 2 files changed, 3 insertions(+), 38 deletions(-)

diff --git a/library/thread.closeable_channel.m b/library/thread.closeable_channel.m
index 32b906751..7b9677d83 100644
--- a/library/thread.closeable_channel.m
+++ b/library/thread.closeable_channel.m
@@ -39,17 +39,6 @@
     %
 :- pred close(closeable_channel(T)::in, io::di, io::uo) is det.
 
-    % Return the state of a channel at a point in time. If the return value
-    % is `yes' then the channel is closed, and will remain closed.
-    % If the return value is `no' then the channel was not yet closed,
-    % but by the time you inspect the value the channel could be closed by
-    % another thread.
-    %
-    % We (the Mercury developers) are unsure if this predicate will be useful
-    % in practice. If you do find a use for it, please let us know.
-    %
-:- pred is_closed(closeable_channel(T)::in, bool::out, io::di, io::uo) is det.
-
 :- type take_result(T)
     --->    ok(T)
     ;       closed.
@@ -127,27 +116,12 @@ put(channel(_Read, Write), Val, Success, !IO) :-
 
 close(channel(_Read, Write), !IO) :-
     mvar.take(Write, Hole, !IO),
+    % We have exclusive WRITE access to the hole.
+    % If the channel is open, the hole is empty so this will succeed.
+    % If the channel is closed, the hole is already full with `closed'.
     mvar.try_put(Hole, closed, _Success, !IO),
     mvar.put(Write, Hole, !IO).
 
-is_closed(channel(_Read, Write), IsClosed, !IO) :-
-    mvar.take(Write, Hole, !IO),
-    mvar.try_read(Hole, HoleFull, !IO),
-    (
-        HoleFull = yes(ItemOrClosed),
-        (
-            ItemOrClosed = closed,
-            IsClosed = yes
-        ;
-            ItemOrClosed = item(_, _),
-            unexpected($pred, "open channel has full hole at write end")
-        )
-    ;
-        HoleFull = no,
-        IsClosed = no
-    ),
-    mvar.put(Write, Hole, !IO).
-
 take(channel(Read, _Write), Res, !IO) :-
     mvar.take(Read, Head, !IO),
     mvar.read(Head, ItemOrClosed, !IO),
diff --git a/tests/hard_coded/closeable_channel_test.m b/tests/hard_coded/closeable_channel_test.m
index cf747c7e0..2a8ebc1cf 100644
--- a/tests/hard_coded/closeable_channel_test.m
+++ b/tests/hard_coded/closeable_channel_test.m
@@ -75,15 +75,6 @@ run_test(!IO) :-
     % Close open channel.
     trace_close(Log, InCh, !IO),
 
-    % Check is_closed.
-    is_closed(InCh, IsClosed, !IO),
-    (
-        IsClosed = yes
-    ;
-        IsClosed = no,
-        log(Log, "ERROR: channel not closed", !IO)
-    ),
-
     % Wait for threads to exit.
     log(Log, "waiting for worker threads to exit", !IO),
     list.foldl(semaphore.wait, DoneSems, !IO),
-- 
2.25.0



More information about the reviews mailing list