8339850: Restore the interrupt status in FileSystemPreferences.lockFile() #20938

sbgoog · 2024-09-10T16:37:11Z

FIleSystemPreferences.lockFile() catches an InterruptedException and just returns false, forgetting to re-interrupt the current thread. This leaves the caller with no way to observe that the thread was interrupted. This change restores the interrupted status on the current thread before returning.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8339850: Restore the interrupt status in FileSystemPreferences.lockFile() (Bug - P4)

Reviewers

Brian Burkhalter (@bplb - Reviewer)
Daniel Jeliński (@djelinski - Reviewer)
Vyom Tewari (@vyommani - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20938

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-09-10T16:38:07Z

👋 Welcome back sbgoog! 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 · 2024-09-10T16:38:50Z

@sbgoog This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8339850: Restore the interrupt status in FileSystemPreferences.lockFile()

Reviewed-by: bpb, djelinski, vtewari

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 308 new commits pushed to the master branch:

49501fe: 8341412: Various test failures after JDK-8334305
8838048: 8324259: Classes used by CDS at runtime should be archived
5e98007: 8307532: Implement LM_LIGHTWEIGHT for Zero
855c8a7: 8334305: Remove all code for nsk.share.Log verbose mode
d6820d1: 8336274: MutableBigInteger.leftShift(int) optimization
a4ca626: 8341146: RISC-V: Unnecessary fences used for load-acquire in template interpreter
c8c4ff2: 8341135: Incorrect format string after JDK-8339475
efe3573: 8340109: Ubsan: ciEnv.cpp:1660:65: runtime error: member call on null pointer of type 'struct CompileTask'
52c2ea6: 8340732: RISC-V: Refactor crc32 scalar version
90c944f: 8340824: C2: Memory for TypeInterfaces not reclaimed by hashcons()
... and 298 more: https://git.openjdk.org/jdk/compare/be0dca046a43ecef2dcd012da6399cbed4cd0454...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@bplb, @djelinski, @vyommani) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2024-09-10T16:39:29Z

@sbgoog 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.

mlbridge · 2024-09-10T16:42:15Z

Webrevs

bplb · 2024-09-12T21:04:16Z

/reviewers 2 reviewer

openjdk · 2024-09-12T21:04:39Z

@bplb
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

bplb

Looks all right.

sbgoog · 2024-09-27T14:38:56Z

This PR needs another reviewer. Is there someone that can have a look at it?

bplb · 2024-09-27T15:48:00Z

This PR needs another reviewer. Is there someone that can have a look at it?

@djelinski ? @jaikiran ?

djelinski · 2024-09-27T18:46:54Z

src/java.prefs/unix/classes/java/util/prefs/FileSystemPreferences.java

@@ -958,6 +958,8 @@ private boolean lockFile(boolean shared) throws SecurityException{
                Thread.sleep(sleepTime);
            } catch(InterruptedException e) {
                checkLockFile0ErrorCode(errorCode);
+                // Don't lose the interrupt unless we throw.


Why should we lose the interrupted status if we throw a SecurityException? It doesn't look right.

I thought that if an access error is found, then that would take precedence over the interrupt, especially since it occurs before the sleep. I was more concerned with just returning false and the caller not knowing if it's false due to interrupt, or false because the file could not be locked after all retries.

Certainly the interrupt status could move before the check which may through, but I think it's less likely that the status will be checked in a catch of SecurityException.

The status might not be explicitly checked, but setting the interrupted status will make sure that subsequent calls to sleep/await/tryLock etc. will not block.

In general, we want to preserve the interrupted status until either the user decides that it's fine to clear, or until the thread dies.

In which case the code might be simplified to just:

} catch (InterruptedException e) { // Don't lose the interrupt Thread.currentThread().interrupt(); break; }

I've reworked the change to always set the interrupted status. I wouldn't remove the check of the error code here, as it'd be a behavior change. I can follow up with that, though it seems to me that it's good to still have the check for access error here.

My suggestion was to break; instead of return false; which would fall through to line 967 where the check method would be called. But what you have is probably preferable as it preserves the original style and keep the changes minimal.

…le()

openjdk · 2024-09-30T14:27:57Z

@sbgoog Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

djelinski · 2024-09-30T15:58:53Z

For future reference, as the bot says, don't force push. The bot will squash all commits before merging.

The change looks good to me. Please update the copyright year.

sbgoog · 2024-09-30T16:08:19Z

For future reference, as the bot says, don't force push. The bot will squash all commits before merging.

I didn't realize before pushing, but now I know. Thanks!

The change looks good to me. Please update the copyright year.

I've done that now.

vyommani

Looks OK

sbgoog · 2024-10-02T15:12:28Z

/integrate

openjdk · 2024-10-02T15:14:51Z

@sbgoog
Your change (at version 2c0c189) is now ready to be sponsored by a Committer.

cushon · 2024-10-02T15:27:52Z

/sponsor

openjdk · 2024-10-02T15:30:24Z

Going to push as commit 9fc1c68.
Since your change was applied there have been 310 commits pushed to the master branch:

5063494: 8340785: Update description of PassFailJFrame and samples
85f0442: 8317116: Provide layouts for multiple test UI in PassFailJFrame
49501fe: 8341412: Various test failures after JDK-8334305
8838048: 8324259: Classes used by CDS at runtime should be archived
5e98007: 8307532: Implement LM_LIGHTWEIGHT for Zero
855c8a7: 8334305: Remove all code for nsk.share.Log verbose mode
d6820d1: 8336274: MutableBigInteger.leftShift(int) optimization
a4ca626: 8341146: RISC-V: Unnecessary fences used for load-acquire in template interpreter
c8c4ff2: 8341135: Incorrect format string after JDK-8339475
efe3573: 8340109: Ubsan: ciEnv.cpp:1660:65: runtime error: member call on null pointer of type 'struct CompileTask'
... and 300 more: https://git.openjdk.org/jdk/compare/be0dca046a43ecef2dcd012da6399cbed4cd0454...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-02T15:30:36Z

@cushon @sbgoog Pushed as commit 9fc1c68.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk bot added the rfr Pull request is ready for review label Sep 10, 2024

openjdk bot added the core-libs core-libs-dev@openjdk.org label Sep 10, 2024

bplb approved these changes Sep 12, 2024

View reviewed changes

djelinski reviewed Sep 27, 2024

View reviewed changes

8339850: Restore the interrupt status in FileSystemPreferences.lockFi…

0b03041

…le()

sbgoog force-pushed the JDK-8339850-restore-interrupt branch from 27cba44 to 0b03041 Compare September 30, 2024 14:26

Update Copyright year

2c0c189

djelinski approved these changes Oct 1, 2024

View reviewed changes

bplb approved these changes Oct 1, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 1, 2024

vyommani approved these changes Oct 2, 2024

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 2, 2024

openjdk bot added the integrated Pull request has been integrated label Oct 2, 2024

openjdk bot closed this Oct 2, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Oct 2, 2024

openjdk bot removed rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Oct 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8339850: Restore the interrupt status in FileSystemPreferences.lockFile() #20938

8339850: Restore the interrupt status in FileSystemPreferences.lockFile() #20938

sbgoog commented Sep 10, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 10, 2024

openjdk bot commented Sep 10, 2024 •

edited

Loading

openjdk bot commented Sep 10, 2024

mlbridge bot commented Sep 10, 2024 •

edited

Loading

bplb commented Sep 12, 2024

openjdk bot commented Sep 12, 2024

bplb left a comment

sbgoog commented Sep 27, 2024

bplb commented Sep 27, 2024

djelinski Sep 27, 2024

sbgoog Sep 30, 2024

djelinski Sep 30, 2024

dfuch Sep 30, 2024

sbgoog Sep 30, 2024

dfuch Sep 30, 2024

openjdk bot commented Sep 30, 2024

djelinski commented Sep 30, 2024

sbgoog commented Sep 30, 2024

vyommani left a comment

sbgoog commented Oct 2, 2024

openjdk bot commented Oct 2, 2024

cushon commented Oct 2, 2024

openjdk bot commented Oct 2, 2024

openjdk bot commented Oct 2, 2024

8339850: Restore the interrupt status in FileSystemPreferences.lockFile() #20938

8339850: Restore the interrupt status in FileSystemPreferences.lockFile() #20938

Conversation

sbgoog commented Sep 10, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 10, 2024

openjdk bot commented Sep 10, 2024 • edited Loading

openjdk bot commented Sep 10, 2024

mlbridge bot commented Sep 10, 2024 • edited Loading

Webrevs

bplb commented Sep 12, 2024

openjdk bot commented Sep 12, 2024

bplb left a comment

Choose a reason for hiding this comment

sbgoog commented Sep 27, 2024

bplb commented Sep 27, 2024

djelinski Sep 27, 2024

Choose a reason for hiding this comment

sbgoog Sep 30, 2024

Choose a reason for hiding this comment

djelinski Sep 30, 2024

Choose a reason for hiding this comment

dfuch Sep 30, 2024

Choose a reason for hiding this comment

sbgoog Sep 30, 2024

Choose a reason for hiding this comment

dfuch Sep 30, 2024

Choose a reason for hiding this comment

openjdk bot commented Sep 30, 2024

djelinski commented Sep 30, 2024

sbgoog commented Sep 30, 2024

vyommani left a comment

Choose a reason for hiding this comment

sbgoog commented Oct 2, 2024

openjdk bot commented Oct 2, 2024

cushon commented Oct 2, 2024

openjdk bot commented Oct 2, 2024

openjdk bot commented Oct 2, 2024

sbgoog commented Sep 10, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 10, 2024 •

edited

Loading

mlbridge bot commented Sep 10, 2024 •

edited

Loading