Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v2]
On Fri, 8 Sep 2023 18:45:02 GMT, Brent Christian wrote: > I would also like to see what a FileChannel implementation looks like. Am I > right that this would allow the `prefs` native library to be removed entirely? Yes. FileChannel.open can be called with the file permissions to atomically set when creating the lock file, and the 3-arg lock method can be used for both shared and exclusive locking. The usages of chmod would change to Files.setPosixFilePermissions. It should be quick to try to see if there are any issues. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1712458404
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v2]
On Fri, 8 Sep 2023 08:03:51 GMT, Alan Bateman wrote: > > We could do this mid-term in some follow up action. > > It shouldn't be hard to try it. If it works out then it would mean this old > crufty code goes away and the JDK is in a better place. If it doesn't work > out then the a plan B would be to add to have lockFile0 throw with a useful > exception. I would also like to see what a FileChannel implementation looks like. Am I right that this would allow the `prefs` native library to be removed entirely? - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1712087144
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v2]
On Fri, 8 Sep 2023 07:44:21 GMT, Matthias Baesken wrote: > We could do this mid-term in some follow up action. It shouldn't be hard to try it. If it works out then it would mean this old crufty code goes away and the JDK is in a better place. If it doesn't work out then the a plan B would be to add to have lockFile0 throw with a useful exception. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1711250512
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v2]
On Fri, 8 Sep 2023 07:19:55 GMT, Alan Bateman wrote: > we shouldn't be hardcoding the values of error numbers like this. Then print the good old int value and rely on OS tools to convert this, it is rather easy and available on some distros). Or move the current error-code handling code from nio to java.base (we have already coding for this in the JDK but cannot use it). > replace the Unix implementation so that it uses FileChannel/Files We could do this mid-term in some follow up action. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1711225903
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v2]
On Mon, 4 Sep 2023 11:57:47 GMT, Matthias Baesken wrote: >>> Hi Alan , Your assumption 'I assume the use of System.getProperty is >>> problematic when running with a SM.' is most likely correct. >> >> You'll need to test with a SM that denies reading the system property to be >> sure. There are classes in many tool APIs (you've listed some) where this >> doesn't arise. >> >> In any case, I assume this lookup will go away once you replace this method. > >> > Hi Alan , Your assumption 'I assume the use of System.getProperty is >> > problematic when running with a SM.' is most likely correct. >> >> You'll need to test with a SM that denies reading the system property to be >> sure. There are classes in many tool APIs (you've listed some) where this >> doesn't arise. >> > > For > > java.management/share/classes/sun/management/VMManagementImpl.java > > I tried with a security manager and had to grant property access so that it > worked properly. > > For > > java.desktop/share/classes/sun/awt/FontConfiguration.java > > this can be called from all code working with fonts so it needs to be handled > too (e.g. PrivilegedAction). > > Should I file a JBS issue for those two ? > > > Some of the others listed might indeed fall into the tool APIs you mentioned. @MBaesken I don't think the changes in this PR should be integrated, we shouldn't be hardcoding the values of error numbers like this. Can you think again about the suggestion to replace the Unix implementation so that it uses FileChannel/Files? That should allow you to remove the native methods for locking and chmod. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1711197140
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v5]
> We run into some BackingStoreException: Couldn't get file lock. e.g. here : > > [JShell] Exception in thread "main" java.lang.IllegalStateException: > java.util.prefs.BackingStoreException: Couldn't get file lock. > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) > [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file > lock. > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) > [JShell] ... 4 more > > The BackingStoreException should be enhanced e.g. by adding the > errno/errorCode that is already available in the coding Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: remove unneeded empty line - Changes: - all: https://git.openjdk.org/jdk/pull/15308/files - new: https://git.openjdk.org/jdk/pull/15308/files/b63064ec..5e3108b8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15308&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15308&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15308.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15308/head:pull/15308 PR: https://git.openjdk.org/jdk/pull/15308
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v4]
On Thu, 7 Sep 2023 14:28:47 GMT, Roger Riggs wrote: >> Matthias Baesken has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Use PrivilegedAction to get OS name > > src/java.prefs/unix/classes/java/util/prefs/FileSystemPreferences.java line > 36: > >> 34: import sun.util.logging.PlatformLogger; >> 35: >> 36: > > Extra blank line. Hi Roger, I removed the line, - PR Review Comment: https://git.openjdk.org/jdk/pull/15308#discussion_r1319454172
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v4]
On Mon, 4 Sep 2023 11:01:06 GMT, Matthias Baesken wrote: >> We run into some BackingStoreException: Couldn't get file lock. e.g. here : >> >> [JShell] Exception in thread "main" java.lang.IllegalStateException: >> java.util.prefs.BackingStoreException: Couldn't get file lock. >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) >> [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file >> lock. >> [JShell] at >> java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) >> [JShell] at >> java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) >> [JShell] ... 4 more >> >> The BackingStoreException should be enhanced e.g. by adding the >> errno/errorCode that is already available in the coding > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Use PrivilegedAction to get OS name src/java.prefs/unix/classes/java/util/prefs/FileSystemPreferences.java line 36: > 34: import sun.util.logging.PlatformLogger; > 35: > 36: Extra blank line. - PR Review Comment: https://git.openjdk.org/jdk/pull/15308#discussion_r1318702833
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v2]
On Fri, 1 Sep 2023 13:27:58 GMT, Alan Bateman wrote: > > Hi Alan , Your assumption 'I assume the use of System.getProperty is > > problematic when running with a SM.' is most likely correct. > > You'll need to test with a SM that denies reading the system property to be > sure. There are classes in many tool APIs (you've listed some) where this > doesn't arise. > For java.management/share/classes/sun/management/VMManagementImpl.java I tried with a security manager and had to grant property access so that it worked properly. For java.desktop/share/classes/sun/awt/FontConfiguration.java this can be called from all code working with fonts so it needs to be handled too (e.g. PrivilegedAction). Should I file a JBS issue for those two ? Some of the others listed might indeed fall into the tool APIs you mentioned. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1705142502
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v4]
> We run into some BackingStoreException: Couldn't get file lock. e.g. here : > > [JShell] Exception in thread "main" java.lang.IllegalStateException: > java.util.prefs.BackingStoreException: Couldn't get file lock. > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) > [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file > lock. > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) > [JShell] ... 4 more > > The BackingStoreException should be enhanced e.g. by adding the > errno/errorCode that is already available in the coding Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: Use PrivilegedAction to get OS name - Changes: - all: https://git.openjdk.org/jdk/pull/15308/files - new: https://git.openjdk.org/jdk/pull/15308/files/d385dac1..b63064ec Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15308&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15308&range=02-03 Stats: 6 lines in 1 file changed: 4 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15308.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15308/head:pull/15308 PR: https://git.openjdk.org/jdk/pull/15308
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v2]
On Fri, 1 Sep 2023 13:10:19 GMT, Matthias Baesken wrote: > Hi Alan , Your assumption 'I assume the use of System.getProperty is > problematic when running with a SM.' is most likely correct. You'll need to test with a SM that denies reading the system property to be sure. There are classes in many tool APIs (you've listed some) where this doesn't arise. In any case, I assume this lookup will go away once you replace this method. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1702749867
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v2]
On Fri, 1 Sep 2023 12:36:28 GMT, Matthias Baesken wrote: >> We run into some BackingStoreException: Couldn't get file lock. e.g. here : >> >> [JShell] Exception in thread "main" java.lang.IllegalStateException: >> java.util.prefs.BackingStoreException: Couldn't get file lock. >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) >> [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file >> lock. >> [JShell] at >> java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) >> [JShell] at >> java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) >> [JShell] ... 4 more >> >> The BackingStoreException should be enhanced e.g. by adding the >> errno/errorCode that is already available in the coding > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > add for some errnos also the string Hi Martin, thanks for the review ! I did some small adjustments to follow your coding style suggestions. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1702741358
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v3]
> We run into some BackingStoreException: Couldn't get file lock. e.g. here : > > [JShell] Exception in thread "main" java.lang.IllegalStateException: > java.util.prefs.BackingStoreException: Couldn't get file lock. > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) > [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file > lock. > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) > [JShell] ... 4 more > > The BackingStoreException should be enhanced e.g. by adding the > errno/errorCode that is already available in the coding Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: follow Martin's codestyle suggestions - Changes: - all: https://git.openjdk.org/jdk/pull/15308/files - new: https://git.openjdk.org/jdk/pull/15308/files/9230e1e3..d385dac1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15308&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15308&range=01-02 Stats: 4 lines in 1 file changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/15308.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15308/head:pull/15308 PR: https://git.openjdk.org/jdk/pull/15308
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v2]
On Fri, 1 Sep 2023 12:36:28 GMT, Matthias Baesken wrote:
>> We run into some BackingStoreException: Couldn't get file lock. e.g. here :
>>
>> [JShell] Exception in thread "main" java.lang.IllegalStateException:
>> java.util.prefs.BackingStoreException: Couldn't get file lock.
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313)
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692)
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008)
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261)
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120)
>> [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file
>> lock.
>> [JShell] at
>> java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769)
>> [JShell] at
>> java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864)
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311)
>> [JShell] ... 4 more
>>
>> The BackingStoreException should be enhanced e.g. by adding the
>> errno/errorCode that is already available in the coding
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> add for some errnos also the string
Hi Alan ,
Your assumption 'I assume the use of System.getProperty is problematic when
running with a SM.'is most likely correct.
java.desktop/share/classes/sun/awt/FontConfiguration.java-133-protected
void setOsNameAndVersion() {
java.desktop/share/classes/sun/awt/FontConfiguration.java:134:osName =
System.getProperty("os.name");
java.desktop/share/classes/sun/awt/FontConfiguration.java-135-osVersion
= System.getProperty("os.version");
java.desktop/share/classes/sun/awt/FontConfiguration.java-136-}
java.desktop/share/classes/sun/awt/FontConfiguration.java-137-
--
java.management/share/classes/javax/management/loading/MLet.java-1054-
// the architecture-specific path name. e.g. if user
java.management/share/classes/javax/management/loading/MLet.java-1055-
// requested a load for "foo" on Solaris SPARC 5.7 we try to
java.management/share/classes/javax/management/loading/MLet.java-1056-
// load "SunOS/sparc/5.7/lib/libfoo.so" from the JAR file.
java.management/share/classes/javax/management/loading/MLet.java-1057-
//
java.management/share/classes/javax/management/loading/MLet.java:1058:
nativelibname = removeSpace(System.getProperty("os.name")) + File.separator +
java.management/share/classes/javax/management/loading/MLet.java-1059-
removeSpace(System.getProperty("os.arch")) + File.separator +
java.management/share/classes/javax/management/loading/MLet.java-1060-
removeSpace(System.getProperty("os.version")) + File.separator +
java.management/share/classes/javax/management/loading/MLet.java-1061-
"lib" + File.separator + nativelibname;
java.management/share/classes/javax/management/loading/MLet.java-1062-
if (MLET_LOGGER.isLoggable(Level.TRACE)) {
--
java.management/share/classes/sun/management/VMManagementImpl.java-223-//
Operating System
java.management/share/classes/sun/management/VMManagementImpl.java-224-
public String getOsName() {
java.management/share/classes/sun/management/VMManagementImpl.java:225:
return System.getProperty("os.name");
java.management/share/classes/sun/management/VMManagementImpl.java-226-}
java.management/share/classes/sun/management/VMManagementImpl.java-227-
public String getOsArch() {
java.management/share/classes/sun/management/VMManagementImpl.java-228-
return System.getProperty("os.arch");
java.management/share/classes/sun/management/VMManagementImpl.java-229-}
--
jdk.compiler/share/classes/com/sun/tools/javac/jvm/JNIWriter.java-99-
jdk.compiler/share/classes/com/sun/tools/javac/jvm/JNIWriter.java-100-
private Context context;
jdk.compiler/share/classes/com/sun/tools/javac/jvm/JNIWriter.java-101-
jdk.compiler/share/classes/com/sun/tools/javac/jvm/JNIWriter.java-102-
private static final boolean isWindows =
jdk.compiler/share/classes/com/sun/tools/javac/jvm/JNIWriter.java:103:
System.getProperty("os.name").startsWith("Windows");
jdk.compiler/share/classes/com/sun/tools/javac/jvm/JNIWriter.java-104-
--
jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/PlatformInfo.java-29-
jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/PlatformInfo.java-30-public
class PlatformInfo {
jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/PlatformInfo.java-31-
/* Returns "win32" if Windows; "linux" if Linux. */
jdk.hotspot.agent/share/classes/s
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v2]
On Fri, 1 Sep 2023 12:36:28 GMT, Matthias Baesken wrote: >> We run into some BackingStoreException: Couldn't get file lock. e.g. here : >> >> [JShell] Exception in thread "main" java.lang.IllegalStateException: >> java.util.prefs.BackingStoreException: Couldn't get file lock. >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) >> [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file >> lock. >> [JShell] at >> java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) >> [JShell] at >> java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) >> [JShell] ... 4 more >> >> The BackingStoreException should be enhanced e.g. by adding the >> errno/errorCode that is already available in the coding > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > add for some errnos also the string Hi Alan, > If there are only a couple of errno's that occur for these cases, it might be > easier to hardwire the translation. I just followed the idea suggested above to do the translation in the Java class itself . Do you have a good example where it is done by strerror and then passed back to Java , without much overhead? - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1702721259
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v2]
On Fri, 1 Sep 2023 12:36:28 GMT, Matthias Baesken wrote: >> We run into some BackingStoreException: Couldn't get file lock. e.g. here : >> >> [JShell] Exception in thread "main" java.lang.IllegalStateException: >> java.util.prefs.BackingStoreException: Couldn't get file lock. >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) >> [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file >> lock. >> [JShell] at >> java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) >> [JShell] at >> java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) >> [JShell] at >> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) >> [JShell] ... 4 more >> >> The BackingStoreException should be enhanced e.g. by adding the >> errno/errorCode that is already available in the coding > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > add for some errnos also the string src/java.prefs/unix/classes/java/util/prefs/FileSystemPreferences.java line 722: > 720: case 1: return "EPERM"; > 721: } > 722: return ""; I assume the use of System.getProperty is problematic when running with a SM. In any case, I don't think this is the right way to do it, instead you want a native method to call strerror to translate the error. - PR Review Comment: https://git.openjdk.org/jdk/pull/15308#discussion_r1312988133
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v2]
On Fri, 1 Sep 2023 12:36:28 GMT, Matthias Baesken wrote:
>> We run into some BackingStoreException: Couldn't get file lock. e.g. here :
>>
>> [JShell] Exception in thread "main" java.lang.IllegalStateException:
>> java.util.prefs.BackingStoreException: Couldn't get file lock.
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313)
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692)
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008)
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261)
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120)
>> [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file
>> lock.
>> [JShell] at
>> java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769)
>> [JShell] at
>> java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864)
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311)
>> [JShell] ... 4 more
>>
>> The BackingStoreException should be enhanced e.g. by adding the
>> errno/errorCode that is already available in the coding
>
> Matthias Baesken has updated the pull request incrementally with one
> additional commit since the last revision:
>
> add for some errnos also the string
Nice improvement! Please see my minor comments.
src/java.prefs/unix/classes/java/util/prefs/FileSystemPreferences.java line 723:
> 721: }
> 722: return "";
> 723: }
Would be a nice use case of switch expressions. But, I'm ok with it.
src/java.prefs/unix/classes/java/util/prefs/FileSystemPreferences.java line 731:
> 729: if (errCode != 0) {
> 730: String errStr = getErrorString(errCode);
> 731: if (! errStr.equals("")) errStr = " (" + errStr + ")";
Coding style: whitespace after '!'
src/java.prefs/unix/classes/java/util/prefs/FileSystemPreferences.java line 732:
> 730: String errStr = getErrorString(errCode);
> 731: if (! errStr.equals("")) errStr = " (" + errStr + ")";
> 732: throw(new BackingStoreException("Couldn't get file lock.
> errno is " + errCode + errStr));
Strange coding style (also in the original code). I'd use "throw new ...".
src/java.prefs/unix/classes/java/util/prefs/FileSystemPreferences.java line 798:
> 796:if (!shared) sharingMode = "nonshared";
> 797:String errStr = getErrorString(errCode);
> 798:if (! errStr.equals("")) errStr = " (" + errStr + ")";
as above
src/java.prefs/unix/classes/java/util/prefs/FileSystemPreferences.java line 799:
> 797:String errStr = getErrorString(errCode);
> 798:if (! errStr.equals("")) errStr = " (" + errStr + ")";
> 799:throw(new BackingStoreException("Couldn't get file lock.
> errno is " + errCode + errStr + " mode is " + sharingMode));
as above
-
Marked as reviewed by mdoerr (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/15308#pullrequestreview-1606876656
PR Review Comment: https://git.openjdk.org/jdk/pull/15308#discussion_r1312979770
PR Review Comment: https://git.openjdk.org/jdk/pull/15308#discussion_r1312980202
PR Review Comment: https://git.openjdk.org/jdk/pull/15308#discussion_r1312982035
PR Review Comment: https://git.openjdk.org/jdk/pull/15308#discussion_r1312982899
PR Review Comment: https://git.openjdk.org/jdk/pull/15308#discussion_r1312983075
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Wed, 16 Aug 2023 13:36:38 GMT, Matthias Baesken wrote: > We run into some BackingStoreException: Couldn't get file lock. e.g. here : > > [JShell] Exception in thread "main" java.lang.IllegalStateException: > java.util.prefs.BackingStoreException: Couldn't get file lock. > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) > [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file > lock. > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) > [JShell] ... 4 more > > The BackingStoreException should be enhanced e.g. by adding the > errno/errorCode that is already available in the coding I added the output of some errno related strings (like EACCES) to the exceptions. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1702671601
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. [v2]
> We run into some BackingStoreException: Couldn't get file lock. e.g. here : > > [JShell] Exception in thread "main" java.lang.IllegalStateException: > java.util.prefs.BackingStoreException: Couldn't get file lock. > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) > [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file > lock. > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) > [JShell] ... 4 more > > The BackingStoreException should be enhanced e.g. by adding the > errno/errorCode that is already available in the coding Matthias Baesken has updated the pull request incrementally with one additional commit since the last revision: add for some errnos also the string - Changes: - all: https://git.openjdk.org/jdk/pull/15308/files - new: https://git.openjdk.org/jdk/pull/15308/files/49519782..9230e1e3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15308&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15308&range=00-01 Stats: 32 lines in 1 file changed: 26 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/15308.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15308/head:pull/15308 PR: https://git.openjdk.org/jdk/pull/15308
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Mon, 28 Aug 2023 07:28:48 GMT, Matthias Baesken wrote: > Hi Alan , when adding the test group sun/tools/jhsdb in TEST.root to the > exclusiveAccess.dirs , I cannot see the error any more. So it seems your > suggestions is correct . My command was actually wondering why we didn't have a TEST.properties in java/util/prefs to define this property, I didn't spot that it is already defined in TEST.ROOT in the root directory. If that wasn't there then I would have expected to see a lot more issues running these tests concurrently. It sounds like this may be useful for the SA or jhsdb tests too. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1696957881
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Tue, 22 Aug 2023 07:48:46 GMT, Matthias Baesken wrote: > For the tests, I'm surprised the TEST.properties in this directory doesn't > have exclusiveAccess.dirs=. as there has been historical issues with > interference between tests in this area. Hi Alan , when adding the test group sun/tools/jhsdb in TEST.root to the exclusiveAccess.dirs , I cannot see the error any more. So it seems your suggestions is correct . - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1695175709
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
Hello,I don’t really understand this discussion,is the normal IO and NIO api not already interpreting error codes? (I am quite sure I often see nationalized error messages in exceptions).https://github.com/openjdk/jdk/blob/76b9011c9ecb8c0c713a58d034f281ba70d65d4e/src/java.base/unix/classes/sun/nio/fs/UnixException.java#L64I think switching to non-native implementation will not only benefit the error messages.GrussBernd-- http://bernd.eckenfels.net Von: core-libs-dev im Auftrag von Matthias Baesken Gesendet: Donnerstag, August 24, 2023 1:47 PMAn: core-libs-dev@openjdk.org Betreff: Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock. On Wed, 16 Aug 2023 13:36:38 GMT, Matthias Baesken wrote: > We run into some BackingStoreException: Couldn't get file lock. e.g. here : > > [JShell] Exception in thread "main" java.lang.IllegalStateException: java.util.prefs.BackingStoreException: Couldn't get file lock. > [JShell] at jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) > [JShell] at jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) > [JShell] at jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) > [JShell] at jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) > [JShell] at jdk.jshell/jdk.internal.jshell.tool.JShellToolProvidermain(JShellToolProvider.java:120) > [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file lock. > [JShell] at java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) > [JShell] at java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) > [JShell] at jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) > [JShell] ... 4 more > > The BackingStoreException should be enhanced e.g. by adding the errno/errorCode that is already available in the coding Btw printing just the errno - integer code looks not so nice first but on a lot of distros there are already OS tools that convert it for you so it is easy to get the string from the int value. :~$ apt install moreutils :~$ errno 11 EAGAIN 11 Resource temporarily unavailable Having said that , I could still hardwire a couple of important errnos on Java level like Roger suggested > If there are only a couple of errno's that occur for these cases, it might be easier to hardwire the translation. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1691522291
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Wed, 16 Aug 2023 13:36:38 GMT, Matthias Baesken wrote: > We run into some BackingStoreException: Couldn't get file lock. e.g. here : > > [JShell] Exception in thread "main" java.lang.IllegalStateException: > java.util.prefs.BackingStoreException: Couldn't get file lock. > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) > [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file > lock. > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) > [JShell] ... 4 more > > The BackingStoreException should be enhanced e.g. by adding the > errno/errorCode that is already available in the coding Btw printing just the errno - integer code looks not so nice first but on a lot of distros there are already OS tools that convert it for you so it is easy to get the string from the int value. :~$ apt install moreutils :~$ errno 11 EAGAIN 11 Resource temporarily unavailable Having said that , I could still hardwire a couple of important errnos on Java level like Roger suggested > If there are only a couple of errno's that occur for these cases, it might be > easier to hardwire the translation. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1691522291
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Wed, 23 Aug 2023 13:02:37 GMT, Matthias Baesken wrote: > This might be the case, but it is a separate issue. This issue is about > improving the BackingStoreException . Normally I would not even touch the > prefs C code, but converting the errno int into something more 'nice' has to > be done somewhere. The file operations already do this. All I'm saying is that an option here is to change FileSystemPreferences to use FileChannel as that gets rid of the problematic native code that you are wrestling with. The result should be a BackingStoreException with a better exception message if there is an error creating the file lock or the lock op fails. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1689961219
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Tue, 22 Aug 2023 07:48:46 GMT, Matthias Baesken wrote: > Hi Alan, should we maybe add this ? If so in this change on in another one ? I don't have a strong opinion on this, I was just surprised to see these tests run concurrently given the history of issues. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1689922379
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Wed, 23 Aug 2023 12:51:30 GMT, Alan Bateman wrote: >I suspect the Unix implementation of prefs can easily be replaced with code >that uses FileChannel. I think it would be good to try > this and would allow >the old native code to be removed. This might be the case, but it is a separate issue. This issue is about improving the BackingStoreException . Normally I would not even touch the prefs C code, but converting the errno int into something more 'nice' has to be done somewhere. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1689924983
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Wed, 23 Aug 2023 12:46:28 GMT, Matthias Baesken wrote: > Hi Alan, should this be done similar to what is done here , using strerror + > strdup and throwing an exception with the generated error message ? > https://github.com/openjdk/jdk/blob/master/src/jdk.attach/linux/native/libattach/VirtualMachineImpl.c#L105 > Does not look very nice to me, but would probably work. I suspect the Unix implementation of prefs can easily be replaced with code that uses FileChannel. I think it would be good to try this and would allow the old native code to be removed. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1689907432
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Fri, 18 Aug 2023 12:35:46 GMT, Alan Bateman wrote: >No, I don't think the legacy prefs API being coupled to internals like this. >Instead I think this is case where the prefs natives >will need to use >strerror. Hi Alan, should this be done similar to what is done here , using strerror + strdup and throwing an exception with the generated error message ? https://github.com/openjdk/jdk/blob/master/src/jdk.attach/linux/native/libattach/VirtualMachineImpl.c#L105 Does not look very nice to me, but would probably work. It is really a bit frustrating that we have already in the OpenJDK codebase 2 solutions (HS runtime/os.cpp and nio UnixConstants ) but none are really usable in other parts of the coding. Looks like a bad duplication (or worse) of efforts. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1689900122
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Mon, 21 Aug 2023 07:14:28 GMT, Alan Bateman wrote: > > How long is the file-lock typically held? How many such tests can run > > concurrently? And how long do we retry for? > > It's typically the sync method when writing back the cached changes. I > suspect the Unix implementation could be easily re-written to use FileChannel > and Files.setPosixFilePermissions and that would eliminate the native code > and give better exception messages when there are errors. > > For the tests, I'm surprised the TEST.properties in this directory doesn't > have exclusiveAccess.dirs=. as there has been historical issues with > interference between tests in this area. Hi Alan, should we maybe add this ? If so in this change on in another one ? - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1687654412
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Mon, 21 Aug 2023 06:48:18 GMT, David Holmes wrote: > How long is the file-lock typically held? How many such tests can run > concurrently? And how long do we retry for? It's typically the sync method when writing back the cached changes. I suspect the Unix implementation could be easily re-written to use FileChannel and Files.setPosixFilePermissions and that would eliminate the native code and give better exception messages when there are errors. For the tests, I'm surprised the TEST.properties in this directory doesn't have exclusiveAccess.dirs=. as there has been historical issues with interference between tests in this area. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1685778195
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Mon, 21 Aug 2023 06:15:07 GMT, Jaikiran Pai wrote: > if multiple tests are running and try to acquire this file lock, then they > might end up interfering with each other, but it's surprising that in some > cases even after multiple attempts the file lock doesn't become available. How long is the file-lock typically held? How many such tests can run concurrently? And how long do we retry for? - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1685748176
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Wed, 16 Aug 2023 13:36:38 GMT, Matthias Baesken wrote: > We run into some BackingStoreException: Couldn't get file lock. e.g. here : > > [JShell] Exception in thread "main" java.lang.IllegalStateException: > java.util.prefs.BackingStoreException: Couldn't get file lock. > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) > [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file > lock. > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) > [JShell] ... 4 more > > The BackingStoreException should be enhanced e.g. by adding the > errno/errorCode that is already available in the coding Hello David, > > errno 11 seems to be EAGAIN. > > Sounds like the native code should be retrying in this case. The Java side of this code does have retry logic which will end up reattempting it for a few times (with sleep() in between) https://github.com/openjdk/jdk/blob/master/src/java.prefs/unix/classes/java/util/prefs/FileSystemPreferences.java#L939. It's possible that if multiple tests are running and try to acquire this file lock, then they might end up interfering with each other, but it's surprising that in some cases even after multiple attempts the file lock doesn't become available. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1685713819
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Fri, 18 Aug 2023 12:30:45 GMT, Matthias Baesken wrote: > errno 11 seems to be EAGAIN. Sounds like the native code should be retrying in this case. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1685653886
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Fri, 18 Aug 2023 12:02:05 GMT, Matthias Baesken wrote: > I checked the usage of sun/nio/fs/UnixConstants.java and seems we would need > to make the class public and an additional export for this like > > ``` > exports sun.nio.fs to > +java.prefs, > jdk.net; > ``` > > would this be acceptable ? No, I don't think the legacy prefs API being coupled to internals like this. Instead I think this is case where the prefs natives will need to use strerror. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1683856641
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Fri, 18 Aug 2023 12:14:35 GMT, Jaikiran Pai wrote: > Hello Matthias, I think adding some diagnostics in this class would be a good > thing. Like in the JBS issue that this PR links to, there's another JBS issue > https://bugs.openjdk.org/browse/JDK-8304938 where we run into intermittent > failures because of this exception. > > However, before doing the changes being proposed in this PR, would it be > better to do those (minimal) changes in a private build and run those tests > and see if these proposed error number or error message diagnostics do > provide the needed detail to narrow down this issue? That might provide > insight on what else (if anything) would need to be included in that > diagnostic message. > > In the meantime, I'll run your proposed change which shows the error number, > just to see if that tells us what's going on with these tests, when they fail. the enhanced output in a failing sun/tools/jhsdb/JStackStressTest.java is java.util.prefs.BackingStoreException: Couldn't get file lock. errno is 11 mode is nonshared errno 11 seems to be EAGAIN. Unfortunately even the enhanced message does not tell me if it is a failure of open or fcntl in the native layer , that would be good to know. We see this issue mostly on a particular Linux machine, probably I should ask the admins what's 'special' about this machine - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1683850190
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Wed, 16 Aug 2023 13:36:38 GMT, Matthias Baesken wrote: > We run into some BackingStoreException: Couldn't get file lock. e.g. here : > > [JShell] Exception in thread "main" java.lang.IllegalStateException: > java.util.prefs.BackingStoreException: Couldn't get file lock. > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) > [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file > lock. > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) > [JShell] ... 4 more > > The BackingStoreException should be enhanced e.g. by adding the > errno/errorCode that is already available in the coding Hello Matthias, I think adding some diagnostics in this class would be a good thing. Like in the JBS issue that this PR links to, there's another JBS issue https://bugs.openjdk.org/browse/JDK-8304938 where we run into intermittent failures because of this exception. However, before doing the changes being proposed in this PR, would it be better to do those (minimal) changes in a private build and run those tests and see if these proposed error number or error message diagnostics do provide the needed detail to narrow down this issue? That might provide insight on what else (if anything) would need to be included in that diagnostic message. In the meantime, I'll run your proposed change which shows the error number, just to see if that tells us what's going on with these tests, when they fail. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1683831684
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Wed, 16 Aug 2023 13:36:38 GMT, Matthias Baesken wrote: > We run into some BackingStoreException: Couldn't get file lock. e.g. here : > > [JShell] Exception in thread "main" java.lang.IllegalStateException: > java.util.prefs.BackingStoreException: Couldn't get file lock. > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) > [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file > lock. > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) > [JShell] ... 4 more > > The BackingStoreException should be enhanced e.g. by adding the > errno/errorCode that is already available in the coding I checked the usage of sun/nio/fs/UnixConstants.java and seems we would need to make the class public and an additional export for this like exports sun.nio.fs to +java.prefs, jdk.net; would this be acceptable ? I could image there are more places in the JDK codebase where this might be interesting. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1683817599
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Thu, 17 Aug 2023 19:50:56 GMT, Roger Riggs wrote: >> Hi Roger, sounds like a good idea to translate it to a more informative >> string. >> Do you have an utility function in mind that does this ? > > Not trivial in java, there are platform specific native functions to get the > string for the current errno. > The function definition for `getLastErrorString` is in jni_uti.h, returning a > java String. > > If there are only a couple of errno's that occur for these cases, it might be > easier to hardwire the translation. > Otherwise, an appropriate native method call and implementation could be > added to FileSystemPreferences.c for both unix and windows. Hi Roger , seems we can get errno numbers at least from open and fcntl , see Java_java_util_prefs_FileSystemPreferences_lockFile0 https://github.com/openjdk/jdk/blob/master/src/java.prefs/unix/native/libprefs/FileSystemPreferences.c#L70 Those functions have quite a few potential errnos, from the Linux manpages at least those https://man7.org/linux/man-pages/man2/open.2.html https://man7.org/linux/man-pages/man2/fcntl.2.html EACCES EAGAIN EBADF EBUSY EDEADLK EDQUOT EEXIST EFAULT EFBIG See EOVERFLOW. EINTR EINVAL EISDIR ELOOP EMFILE ENAMETOOLONG ENFILE ENODEV ENOENT ENOLCK ENOMEM ENOSPC ENOTDIR ENXIO EOPNOTSUPP EOVERFLOW EPERM EROFS ETXTBSY EWOULDBLOCK I noticed src/java.base/unix/classes/sun/nio/fs/UnixConstants.java.template contains some of them, should I add the missing ones there and reference from there ? Btw. when looking at Java_java_util_prefs_FileSystemPreferences_lockFile0 should we better initialize `int result[2]` https://github.com/openjdk/jdk/blob/master/src/java.prefs/unix/native/libprefs/FileSystemPreferences.c#L74 There seems to be a codepath where result[1] is never written but at the end used to fill javeResult https://github.com/openjdk/jdk/blob/master/src/java.prefs/unix/native/libprefs/FileSystemPreferences.c#L117 this looks a bit problematic ? - PR Review Comment: https://git.openjdk.org/jdk/pull/15308#discussion_r1298136160
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Thu, 17 Aug 2023 07:54:30 GMT, Matthias Baesken wrote:
>> src/java.prefs/unix/classes/java/util/prefs/FileSystemPreferences.java line
>> 773:
>>
>>> 771:String sharingMode = "shared";
>>> 772:if (!shared) sharingMode = "nonshared";
>>> 773:throw(new BackingStoreException("Couldn't get file
>>> lock. errno is " + errCode + " mode is " + sharingMode));
>>
>> A raw errno is pretty uninformative, except for experts. Can the errno be
>> translated to a string as well as supplying the errno?
>
> Hi Roger, sounds like a good idea to translate it to a more informative
> string.
> Do you have an utility function in mind that does this ?
Not trivial in java, there are platform specific native functions to get the
string for the current errno.
The function definition for `getLastErrorString` is in jni_uti.h, returning a
java String.
If there are only a couple of errno's that occur for these cases, it might be
easier to hardwire the translation.
Otherwise, an appropriate native method call and implementation could be added
to FileSystemPreferences.c for both unix and windows.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15308#discussion_r1297672699
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Wed, 16 Aug 2023 19:02:52 GMT, Roger Riggs wrote:
>> We run into some BackingStoreException: Couldn't get file lock. e.g. here :
>>
>> [JShell] Exception in thread "main" java.lang.IllegalStateException:
>> java.util.prefs.BackingStoreException: Couldn't get file lock.
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313)
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692)
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008)
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261)
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120)
>> [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file
>> lock.
>> [JShell] at
>> java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769)
>> [JShell] at
>> java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864)
>> [JShell] at
>> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311)
>> [JShell] ... 4 more
>>
>> The BackingStoreException should be enhanced e.g. by adding the
>> errno/errorCode that is already available in the coding
>
> src/java.prefs/unix/classes/java/util/prefs/FileSystemPreferences.java line
> 773:
>
>> 771:String sharingMode = "shared";
>> 772:if (!shared) sharingMode = "nonshared";
>> 773:throw(new BackingStoreException("Couldn't get file lock.
>> errno is " + errCode + " mode is " + sharingMode));
>
> A raw errno is pretty uninformative, except for experts. Can the errno be
> translated to a string as well as supplying the errno?
Hi Roger, sounds like a good idea to translate it to a more informative
string.
Do you have an utility function in mind that does this ?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15308#discussion_r1296822489
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Wed, 16 Aug 2023 13:36:38 GMT, Matthias Baesken wrote:
> We run into some BackingStoreException: Couldn't get file lock. e.g. here :
>
> [JShell] Exception in thread "main" java.lang.IllegalStateException:
> java.util.prefs.BackingStoreException: Couldn't get file lock.
> [JShell] at
> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313)
> [JShell] at
> jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692)
> [JShell] at
> jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008)
> [JShell] at
> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261)
> [JShell] at
> jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120)
> [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file
> lock.
> [JShell] at
> java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769)
> [JShell] at
> java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864)
> [JShell] at
> jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311)
> [JShell] ... 4 more
>
> The BackingStoreException should be enhanced e.g. by adding the
> errno/errorCode that is already available in the coding
src/java.prefs/unix/classes/java/util/prefs/FileSystemPreferences.java line 773:
> 771:String sharingMode = "shared";
> 772:if (!shared) sharingMode = "nonshared";
> 773:throw(new BackingStoreException("Couldn't get file lock.
> errno is " + errCode + " mode is " + sharingMode));
A raw errno is pretty uninformative, except for experts. Can the errno be
translated to a string as well as supplying the errno?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15308#discussion_r1296318043
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Wed, 16 Aug 2023 14:44:23 GMT, Matthias Baesken wrote: > Hi Alan, I am not sure what 'interesting file path' you are referring to. I > only added an integer (errno/errcode) and at one place fix strings > shared/nonshared to the exception message. Oh I see, I thought you wanted to include the file lock on Unix, which would have issues. The error code might be okay, just very different to other places. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1680764222
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Wed, 16 Aug 2023 13:36:38 GMT, Matthias Baesken wrote: > We run into some BackingStoreException: Couldn't get file lock. e.g. here : > > [JShell] Exception in thread "main" java.lang.IllegalStateException: > java.util.prefs.BackingStoreException: Couldn't get file lock. > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) > [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file > lock. > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) > [JShell] ... 4 more > > The BackingStoreException should be enhanced e.g. by adding the > errno/errorCode that is already available in the coding Hi Alan, I am not sure what 'interesting file path' you are referring to. I only added an integer (errno/errcode) and at one place fix strings shared/nonshared to the exception message. Or do you mean something that is already present without my patch ? - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1680752247
Re: RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
On Wed, 16 Aug 2023 13:36:38 GMT, Matthias Baesken wrote: > We run into some BackingStoreException: Couldn't get file lock. e.g. here : > > [JShell] Exception in thread "main" java.lang.IllegalStateException: > java.util.prefs.BackingStoreException: Couldn't get file lock. > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) > [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file > lock. > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) > [JShell] at > java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) > [JShell] at > jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) > [JShell] ... 4 more > > The BackingStoreException should be enhanced e.g. by adding the > errno/errorCode that is already available in the coding I don't object to improving exception message but please try to keep the code consistent style if you can. I think the main question with this change is whether it's okay from a security point of view as to whether an interesting file path might leak into a stack trace served up by a HTTP server. In other words, this one probably needs security input on whether it should be opt-in with jdk.includeInExceptions security config. It's somewhat awkward here as it's the java.prefs module, a faraway (and little used) module that java.base would rather not export anything to. - PR Comment: https://git.openjdk.org/jdk/pull/15308#issuecomment-1680742906
RFR: JDK-8314272: Improve java.util.prefs.BackingStoreException: Couldn't get file lock.
We run into some BackingStoreException: Couldn't get file lock. e.g. here : [JShell] Exception in thread "main" java.lang.IllegalStateException: java.util.prefs.BackingStoreException: Couldn't get file lock. [JShell] at jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:313) [JShell] at jdk.jshell/jdk.internal.jshell.tool.JShellTool$ReplayableHistory.storeHistory(JShellTool.java:692) [JShell] at jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:1008) [JShell] at jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261) [JShell] at jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120) [JShell] Caused by: java.util.prefs.BackingStoreException: Couldn't get file lock. [JShell] at java.prefs/java.util.prefs.FileSystemPreferences.sync(FileSystemPreferences.java:769) [JShell] at java.prefs/java.util.prefs.FileSystemPreferences.flush(FileSystemPreferences.java:864) [JShell] at jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder$PreferencesStorage.flush(JShellToolBuilder.java:311) [JShell] ... 4 more The BackingStoreException should be enhanced e.g. by adding the errno/errorCode that is already available in the coding - Commit messages: - JDK-8314272 Changes: https://git.openjdk.org/jdk/pull/15308/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15308&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8314272 Stats: 15 lines in 1 file changed: 5 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/15308.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15308/head:pull/15308 PR: https://git.openjdk.org/jdk/pull/15308
