Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited
Am 15.04.2011 01:44, schrieb David Holmes: Ulf, Ulf Zibis said the following on 04/15/11 04:02: Oops, SystemRoot could be null theoretically. So forget my comment about NUL termination. But anyway, should we allow to have get(SystemRoot) != getEnv(SystemRoot)? Is it correct to defaultly set the SystemRoot variable on non-Windows OS? This ProcessEnvironment.java is a Windows specific file. Much thanks David for clarification. IMHO there should be a big hint in this file: // // This is the Windows implementation of this class! // // -Ulf
Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited
Ulf Zibis wrote: IMHO there should be a big hint in this file: // // This is the Windows implementation of this class! // // -Ulf I don't think this is needed because all files in the src/windows/** tree are Windows specific. -Alan.
Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited
Am 15.04.2011 10:50, schrieb Alan Bateman: Ulf Zibis wrote: IMHO there should be a big hint in this file: // // This is the Windows implementation of this class! // // -Ulf I don't think this is needed because all files in the src/windows/** tree are Windows specific. Thanks Alan. I had opened the file from JDK's src.zip, the most easy way from NetBeans IDE, and there is no src/windows/** tree. So normal JDK user might be irritated even more if referring to the shipped sources, and if not aware about the branches in hg repository. -Ulf
Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited
Michael McMahon wrote: oops. This isn't correct. Sorry. It seems the NameComparator class uses exactly the same algorithm as the CASE_INSENSITIVE_ORDER comparator provided by the String class. So, it probably makes sense to replace NameComparator with the one from String (one less class in rt.jar :) ) - Michael.
Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited
Michael McMahon wrote: On Windows, we are ensuring that SystemRoot will at a minimum always be set. Don't you still have the corner case where Runtime.exec is invoked with an empty array and SystemRoot is not in the environment (launched by an older JDK for example)? In that case we can't make SystemRoot available to the child process either. -Alan
Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited
Am 15.04.2011 11:54, schrieb Alan Bateman: Michael McMahon wrote: On Windows, we are ensuring that SystemRoot will at a minimum always be set. Don't you still have the corner case where Runtime.exec is invoked with an empty array and SystemRoot is not in the environment (launched by an older JDK for example)? In that case we can't make SystemRoot available to the child process either. ... and then on some versions of MSVCRT.DLL the whole thing will crash ?? Again I ask if we shouldn't disallow to user-define the SystemRoot variable on Windows? Imagine someone has set the variable to C:\WINDOWS, but the correct value is D:\WINDOWS. -Ulf
Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited
Am 15.04.2011 11:45, schrieb Michael McMahon:
Ulf,
On SystemRoot vs systemroot, I had thought that Windows required the sort
to be case-sensitive. But, I double-checked and it doesn't. So we can use
SystemRoot.
If that would be true, we wouldn't need to sort by CASE_INSENSITIVE_ORDER or
rather NameComparator. ;-)
Again my question: What you think of my below code?
I think we only need 1 call site to insert the potentially missing SystemRoot variable, a simple
array is faster than initializing a needless j.u.List, and boolean foundSysRoot is redundant to int cmp.
Additionally I think having the addEnv method is too disproportionate. We can just inline 2
different String-catenations, and it should compile to 3/4 sb.append() under the hood.
So here my shortest version:
// Only for use by ProcessImpl.start()
String toEnvironmentBlock() {
// Sort Unicode-case-insensitively by name
Map.EntryString,String[] list = entrySet().toArray(new
Map.Entry[size()]);
Arrays.sort(list, entryComparator);
StringBuilder sb = new StringBuilder(list.length*30);
for (int i = 0, cmp = -1; ; i++) {
Map.EntryString,String e = i list.length ? list[i] : null;
// Some versions of MSVCRT.DLL require SystemRoot to be set:
final String ROOT = SystemRoot;
if (cmp 0 (e == null || (cmp = nameComparator.compare(e.getKey(),
ROOT)) 0)) {
String value = getenv(ROOT);
if (value != null)
sb.append(ROOT+'='+value+'\u');
// Ensure double NUL termination, even if environment is empty.
else if (list.length == 0)
sb.append('\u');
}
if (e == null) break;
sb.append(e.getKey()+'='+e.getValue()+'\u');
}
// Ensure double NUL termination
return sb.append('\u').toString();
}
-Ulf
Ulf Zibis wrote:
You can have the code much shorter (and faster):
// Only for use by ProcessImpl.start()
String toEnvironmentBlock() {
// Sort Unicode-case-insensitively by name
Map.EntryString,String[] list = entrySet().toArray(new
Map.Entry[size()]);
Arrays.sort(list, entryComparator);
StringBuilder sb = new StringBuilder(list.length*30);
for (int i = 0, cmp = -1; ; i++) {
Map.EntryString,String e = i list.length ? list[i] : null;
final String ROOT = SystemRoot;
if (cmp 0 (e == null || (cmp = nameComparator.compare(e.getKey(),
ROOT)) 0)) {
String value = getenv(ROOT);
if (value != null)
addEnv(sb, ROOT, value);
// Ensure double NUL termination, even if environment is empty.
else if (list.length == 0)
sb.append('\u');
}
if (e == null) break;
addEnv(sb, e.getKey(), e.getValue());
}
// Ensure double NUL termination
return sb.append('\u').toString();
}
// add the environment variable to the child
private static void addEnv(StringBuilder sb, String key, String value) {
sb.append(key+'='+value+'\u'); // under the hood, it compiles to 4
sb.append()
}
Do you like it?
Note: I think, if we use NameComparator, we can use SystemRoot.
-Ulf
Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited
Ulf Zibis wrote: : Again I ask if we shouldn't disallow to user-define the SystemRoot variable on Windows? Imagine someone has set the variable to C:\WINDOWS, but the correct value is D:\WINDOWS. No, I don't think we should silently change the value of this or other variables. -Alan.
Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited
Am 15.04.2011 14:15, schrieb Alan Bateman: Ulf Zibis wrote: : Again I ask if we shouldn't disallow to user-define the SystemRoot variable on Windows? Imagine someone has set the variable to C:\WINDOWS, but the correct value is D:\WINDOWS. No, I don't think we should silently change the value of this or other variables. I don't think about silently changing, I think about to *disallow to set* it e.g. by the put() method, and instead retrieve it by default from constructor, so it could be always read by the get() method. -Ulf
Re: Review request for 7034570 - java.lang.Runtime.exec(String[] cmd, String[] env) can not work properly if SystemRoot not inherited
Ulf Zibis wrote: I don't think about silently changing, I think about to *disallow to set* it e.g. by the put() method, and instead retrieve it by default from constructor, so it could be always read by the get() method. Ah, you mean the environment returned by ProcessBuilder where the spec does allow the Map to forbid changing certain variables. I don't see this working for the Runtime.exec case as we can't change these methods to fail if the environment includes SystemRoot. The other case is where the environment is initially cleared (in ProcessBuilder's class description it has a note suggesting to use the clear method to start a process with an explicit set of environment variables). All told, I think Michael's approach to ensure that SystemRoot is in the child environment is reasonable. -Alan.
Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock
Fix for 7032589: FileHandler leaking file descriptor of the file lock Webrev at: http://cr.openjdk.java.net/~mchung/jdk7/7032589/webrev.00/ When a log file is currently being used, tryLock() will fail and it has to close the file channel of the lock file Thanks Mandy
Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock
Thumbs up. Dan On 4/15/2011 11:01 AM, Mandy Chung wrote: Fix for 7032589: FileHandler leaking file descriptor of the file lock Webrev at: http://cr.openjdk.java.net/~mchung/jdk7/7032589/webrev.00/ When a log file is currently being used, tryLock() will fail and it has to close the file channel of the lock file Thanks Mandy
Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock
On 04/15/2011 07:55 PM, Daniel D. Daugherty wrote: Thumbs up. Dan Hi Mandy, if fc.close() throws an IOException , it goes wrong. I think you need to put fc.close() in its own a try/catch(IOException). Rémi On 4/15/2011 11:01 AM, Mandy Chung wrote: Fix for 7032589: FileHandler leaking file descriptor of the file lock Webrev at: http://cr.openjdk.java.net/~mchung/jdk7/7032589/webrev.00/ When a log file is currently being used, tryLock() will fail and it has to close the file channel of the lock file Thanks Mandy
Review needed for 7037085 : Add hashCode() to Timestamp to address Findbugs warning
Hi all,
Need a reviewer for the following minor change which adds hasCode() to
Timestamp to address a Findbugs warning.
Regards
Lance
hg diff
diff -r d9248245a88c src/share/classes/java/sql/Timestamp.java
--- a/src/share/classes/java/sql/Timestamp.java Wed Apr 13 11:21:36 2011 -0400
+++ b/src/share/classes/java/sql/Timestamp.java Fri Apr 15 14:34:07 2011 -0400
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1996, 2010, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2011, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -515,6 +515,18 @@
}
}
+/**
+ * {@inheritDoc}
+ *
+ * The {@code hashcode} method uses the underlying {@code java.util.Date}
+ * implementation and therefore does not include nanos in its computation.
+ *
+ */
+@Override
+public int hashCode() {
+return super.hashCode();
+}
+
static final long serialVersionUID = 2745179027874758501L;
}
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock
Hi Remi,
On 04/15/11 11:05, Rémi Forax wrote:
Hi Mandy,
if fc.close() throws an IOException , it goes wrong.
I think you need to put fc.close() in its own a try/catch(IOException).
That's a good point. I think it should throw IOException if anything
goes wrong with fc.close(). Revised the fix:
http://cr.openjdk.java.net/~mchung/jdk7/7032589/webrev.01/
The existing implementation looks a bit suspicious as it silently
ignores the IOException thrown by tryLock() and uses that lock file. I
am inclined to leave the existing behavior as it is to minimize
behavioral change. And just resolve this file descriptor leak issue in
this fix. I'll open a bug to follow up this.
412 try {
413 FileLock fl = fc.tryLock();
414 if (fl == null) {
415 // We failed to get the lock. Try next file.
416 continue;
417 }
418 // We got the lock OK.
419 } catch (IOException ix) {
420 // We got an IOException while trying to get the lock.
421 // This normally indicates that locking is not
supported
422 // on the target directory. We have to proceed without
423 // getting a lock. Drop through.
424 }
Mandy
Re: Review needed for 7037085 : Add hashCode() to Timestamp to address Findbugs warning
Hi Eamonn
The javadocs for Timestamp have always specifically called the following blurb
out in the class description. Based on some side discussions, it was best to
also copy this blurb to the added hashCode method (you will see the text at the
top of Timestamp) for additional clarity.
Regards
Lance
On Apr 15, 2011, at 3:37 PM, Eamonn McManus wrote:
This isn't wrong, but wouldn't it be simpler to just add or xor the nanos
field into the hashcode, rather than explicitly saying that you don't?
Éamonn
On 15/4/11 8:54 PM, Lance Andersen - Oracle wrote:
Hi all,
Need a reviewer for the following minor change which adds hasCode() to
Timestamp to address a Findbugs warning.
Regards
Lance
hg diff
diff -r d9248245a88c src/share/classes/java/sql/Timestamp.java
--- a/src/share/classes/java/sql/Timestamp.java Wed Apr 13 11:21:36
2011 -0400
+++ b/src/share/classes/java/sql/Timestamp.java Fri Apr 15 14:34:07
2011 -0400
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1996, 2010, Oracle and/or its affiliates. All rights
reserved.
+ * Copyright (c) 1996, 2011, Oracle and/or its affiliates. All rights
reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -515,6 +515,18 @@
}
}
+/**
+ * {@inheritDoc}
+ *
+ * The {@code hashcode} method uses the underlying {@code
java.util.Date}
+ * implementation and therefore does not include nanos in its
computation.
+ *
+ */
+@Override
+public int hashCode() {
+return super.hashCode();
+}
+
static final long serialVersionUID = 2745179027874758501L;
}
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock
On 4/15/2011 1:12 PM, Mandy Chung wrote:
Hi Remi,
On 04/15/11 11:05, Rémi Forax wrote:
Hi Mandy,
if fc.close() throws an IOException , it goes wrong.
I think you need to put fc.close() in its own a try/catch(IOException).
That's a good point. I think it should throw IOException if anything
goes wrong with fc.close(). Revised the fix:
http://cr.openjdk.java.net/~mchung/jdk7/7032589/webrev.01/
Thumbs up on this version.
The existing implementation looks a bit suspicious as it silently
ignores the IOException thrown by tryLock() and uses that lock file.
I think that the existing code is remembering the lock file name
even which IOOException is thrown because of the assumption that
IOException is only thrown when locking is not supported on the
target directory. By remembering the lock file name, any further
attempt to lock the file by the same process doesn't result in
a another tryLock() call with another resulting IOException.
It is troubling that this code is giving a false assurance that
the file system object is locked when it isn't. In the case where
an IOException was thrown by tryLock(), the file system object is
only locked in the context of the current process. That seems to
make it easy for two different processes to both think that they
have the file system object locked. Potential train wreck down the
road? Quite possibly.
I am inclined to leave the existing behavior as it is to minimize
behavioral change. And just resolve this file descriptor leak issue
in this fix.
Agreed that this bug should address the leak and that a new
bug should be opened for the possible locking issue.
Dan
I'll open a bug to follow up this.
412 try {
413 FileLock fl = fc.tryLock();
414 if (fl == null) {
415 // We failed to get the lock. Try next
file.
416 continue;
417 }
418 // We got the lock OK.
419 } catch (IOException ix) {
420 // We got an IOException while trying to get
the lock.
421 // This normally indicates that locking is
not supported
422 // on the target directory. We have to
proceed without
423 // getting a lock. Drop through.
424 }
Mandy
Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock
On 04/15/11 13:41, Daniel D. Daugherty wrote: On 4/15/2011 1:12 PM, Mandy Chung wrote: Hi Remi, On 04/15/11 11:05, Rémi Forax wrote: Hi Mandy, if fc.close() throws an IOException , it goes wrong. I think you need to put fc.close() in its own a try/catch(IOException). That's a good point. I think it should throw IOException if anything goes wrong with fc.close(). Revised the fix: http://cr.openjdk.java.net/~mchung/jdk7/7032589/webrev.01/ Thumbs up on this version. Thanks for the review. Agreed that this bug should address the leak and that a new bug should be opened for the possible locking issue. I created a CR 7037134: Potential writing to the same log file by multiple processes Mandy
Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock
On 04/15/2011 10:55 PM, Mandy Chung wrote: On 04/15/11 13:41, Daniel D. Daugherty wrote: On 4/15/2011 1:12 PM, Mandy Chung wrote: Hi Remi, On 04/15/11 11:05, Rémi Forax wrote: Hi Mandy, if fc.close() throws an IOException , it goes wrong. I think you need to put fc.close() in its own a try/catch(IOException). That's a good point. I think it should throw IOException if anything goes wrong with fc.close(). Revised the fix: http://cr.openjdk.java.net/~mchung/jdk7/7032589/webrev.01/ Thumbs up on this version. Thanks for the review. Agreed that this bug should address the leak and that a new bug should be opened for the possible locking issue. I created a CR 7037134: Potential writing to the same log file by multiple processes Mandy Hi Mandy, Minor nit, initializing available when declaring it is not necessary because both path (with or without exception) initialize it later. otherwise, I'm Ok with the patch. Rémi
hg: jdk7/tl/jdk: 7035115: sun/security/pkcs11/Provider/ConfigShortPath.java compilation failed
Changeset: 131ed7967996 Author:valeriep Date: 2011-04-15 15:56 -0700 URL: http://hg.openjdk.java.net/jdk7/tl/jdk/rev/131ed7967996 7035115: sun/security/pkcs11/Provider/ConfigShortPath.java compilation failed Summary: Updated the test to use reflection and skip when SunPKCS11 provider not present. Reviewed-by: weijun ! test/sun/security/pkcs11/Provider/ConfigShortPath.java
Re: Review Request (1-line fix): 7032589 FileHandler leaking file descriptor of the file lock
On 4/15/11 3:57 PM, Rémi Forax wrote: Hi Mandy, Minor nit, initializing available when declaring it is not necessary because both path (with or without exception) initialize it later. Sure. Will fix that before the push. otherwise, I'm Ok with the patch. Thanks Mandy
