8349543: LinkedBlockingDeque does not immediately throw InterrruptedException in put/take

[kabutz]

The LinkedBlockingDeque does not behave consistently with other concurrency components. If we call putFirst(), putLast(), takeFirst(), or takeLast() with a thread that is interrupted, it does not immediately throw an InterruptedException, the way that ArrayBlockingQueue and LInkedBlockingQueue does, because instead of lockInterruptibly(), we call lock(). It will only throw an InterruptedException if the queue is full (on put) or empty (on take). Since interruptions are frequently used as a shutdown mechanism, this might prevent code from ever shutting down.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Integration blocker

 ⚠️ Title mismatch between PR and JBS for issue JDK-8349543

Issue

• JDK-8349543: LinkedBlockingDeque does not immediately throw InterruptedException in put/take (Bug - P4) ⚠️ Title mismatch between PR and JBS.

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23464/head:pull/23464
$ git checkout pull/23464

Update a local copy of the PR:
$ git checkout pull/23464
$ git pull https://git.openjdk.org/jdk.git pull/23464/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23464

View PR using the GUI difftool:
$ git pr show -t 23464

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23464.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back kabutz! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@kabutz The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[DougLea]

Thanks for finding this. The only question is whether we believe that any existing usage might depend on current behavior, and if so should it be done anyway?

[mlbridge]

Webrevs

• 00: Full (9a12fe7d)

[kabutz]

Thanks for finding this. The only question is whether we believe that any existing usage might depend on current behavior, and if so should it be done anyway?

Good question - your call Doug.

LinkedBlockingDeque's comment is the same as LInkedBlockingQueue's (inherited from BlockingQueue): "@throws InterruptedException if interrupted while waiting" - however all the other classes that do throw this InterruptedException would do so on entry, even if we are not technically waiting.

Other examples where this behaviour is consistent is LinkedTransferQueue.take(), PriorityBlockingQueue.take(), etc.:

import java.util.concurrent.*;

public class InterruptionsIgnoredOnTakeOtherQueues {
    public static void main(String... args) throws InterruptedException {
        // Ensure that take() immediately throws an InterruptedException
        // if the thread is interrupted for other BlockingQueues
        try (var pool = Executors.newSingleThreadExecutor()) {
            var success = pool.submit(() -> {
                test(new LinkedTransferQueue<>());
                test(new PriorityBlockingQueue<>());
                test(new ArrayBlockingQueue<>(10));
                test(new LinkedBlockingQueue<>());
                return null;
            });
            try {
                success.get();
            } catch (ExecutionException e) {
                try {
                    throw e.getCause();
                } catch (Error | RuntimeException unchecked) {
                    throw unchecked;
                } catch (Throwable cause) {
                    throw new AssertionError(cause);
                }
            }
        }
    }

    private static void test(BlockingQueue<Integer> queue) {
        queue.add(42);
        Thread.currentThread().interrupt();
        try {
            queue.take();
            fail("Expected InterruptedException in " + queue.getClass().getSimpleName() + ".take()");
        } catch (InterruptedException expected) {
            // good that's what we want
        }
    }

    private static void fail(String message) {
        throw new AssertionError(message);
    }
}

[Martin-Buchholz]

I will give this review a bit of my 1% openjdk time

[AlanBateman]

I think this is okay. Anything calling these methods already has to deal with InterruptedException. I see that the implementation offerFirst/offerLast/pollFirst/pollLast is using lockInterruptibly so if usage extends to these methods then there is already the possibility of the exception when calling them with the interrupt status set.

Given the behavior change then a CSR + release notes would be good but I think it's overall.

[bridgekeeper]

@kabutz This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[kabutz]

Should we drop this PR or continue with it?

I noticed that the timed offer() and poll() methods are consistent with the way that LinkedBlockingQueue works, in other words, they throw the InterruptedException as soon as we enter the methods. They use lock.lockInterruptibly(), which I was suggesting we use for put() and take().

import java.util.concurrent.*;

public class OfferPollLBD {
    public static void main(String... args) {
        var lbd = new LinkedBlockingDeque<Integer>();
        Thread.currentThread().interrupt();
        System.out.println(lbd.offer(1)); // should work
        System.out.println(lbd.add(2)); // should work
        try {
            System.out.println(lbd.offer(3, 10, TimeUnit.SECONDS)); // throws IE
        } catch (InterruptedException e) {
            System.out.println("Interrupted");
            Thread.currentThread().interrupt();
        }
        try {
            lbd.put(4); // IMHO should throw IE, but doesn't
            System.out.println("put(4) succeeded");
        } catch (InterruptedException e) {
            System.out.println("Interrupted");
            Thread.currentThread().interrupt();
        }

        System.out.println("lbd = " + lbd);

        System.out.println(lbd.poll()); // should work
        System.out.println(lbd.remove()); // should work
        try {
            System.out.println(lbd.poll(10, TimeUnit.SECONDS)); // throws IE
        } catch (InterruptedException e) {
            System.out.println("Interrupted");
            Thread.currentThread().interrupt();
        }
        try {
            System.out.println(lbd.take());
        } catch (InterruptedException e) {
            System.out.println("Interrupted");
            Thread.currentThread().interrupt();
        }
        System.out.println("lbd = " + lbd);
    }
}

[bridgekeeper]

@kabutz This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[bridgekeeper]

@kabutz This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

[kabutz]

These changes have been included in #24925