Re: RFR: 8277090 : jsr166 refresh for jdk19

[Jason Mehrens]

Hi Alan,

>Is this a Future implementation that doesn't implement the spec
>correctly? The get method shouldn't throw CancellationException if done
>and not-cancelled.

What you say makes sense.  I should have checked this before I brought it up 
but CancellationException is an IllegalStateException so even if it did throw 
CE it is compliant with the docs.


>The intention is that ISE means a coding error. The usage of these
>methods should be very close to the test for the state so I think it's
>right as it is. If you set the cause then it be caught and used like a
>CompletionException.

Bugs sometimes cause other bugs and dropping the chain can hide that 
information about a root problem.  I understand the reluctance to add the cause.

Thanks!

Jason

Re: RFR: 8277090 : jsr166 refresh for jdk19

[Jason Mehrens]

Hi Doug,

In Future::exceptionNow() and Future::state() I would think we would want to 
catch CancellationException since the implementation of the Future is not 
known.  Even though we pre-screen the state I would imagine there could be an 
implementation that prefers cancellation over normal completion.

For Future::resultNow() and FutureTask::resultNow(), is it intentional that we 
are not chaining just ExecutionException::getCause() / (Throwable) outcome as 
the cause of the IllegalStateException?  Chaining that might help with 
debugging even if that is not how it is suppose to be used.

Jason


From: core-libs-dev  on behalf of Doug Lea 

Sent: Sunday, May 1, 2022 7:29 AM
To: core-libs-dev@openjdk.java.net
Subject: RFR: 8277090 :  jsr166 refresh for jdk19

This is the jsr166 refresh for jdk19. See 
https://bugs.openjdk.java.net/browse/JDK-8285450 and 
https://bugs.openjdk.java.net/browse/JDK-8277090

-

Commit messages:
 - whitespace
 - sync with loom
 - 8276202: LogFileOutput.invalid_file_vm asserts when being executed from a 
read only working directory
 - 8284981: Support the vectorization of some counting-down loops in SLP
 - 8284992: Fix misleading Vector API doc for LSHR operator
 - 8254759: [TEST_BUG] [macosx] 
javax/swing/JInternalFrame/4202966/IntFrameCoord.html fails
 - 8285945: [BACKOUT] JDK-8285802 AArch64: Consistently handle offsets in 
MacroAssembler as 64-bit quantities
 - 8284331: Add sanity check for signal handler modification warning.
 - 8285773: Replace Algorithms.eatMemory(...) with WB.fullGC() in 
vmTestbase/gc/gctests/ReferencesGC/ReferencesGC.java
 - 8284883: JVM crash: guarantee(sect->end() <= sect->limit()) failed: sanity 
on AVX512
 - ... and 6 more: https://git.openjdk.java.net/jdk/compare/3d07b3c7...501a21e8

Changes: https://git.openjdk.java.net/jdk/pull/8490/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8490=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8277090
  Stats: 3750 lines in 30 files changed: 2060 ins; 656 del; 1034 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8490.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8490/head:pull/8490

PR: https://git.openjdk.java.net/jdk/pull/8490

Re: fast way to infer caller

[Jason Mehrens]

Perhaps https://bugs.openjdk.java.net/browse/JDK-4515935 for the MemoryHandler 
could be used to determine if StackWalker is fast enough for the lowest rung on 
the StackWalker performance ladder.   Currently the MemoryHandler doesn't not 
infer the caller and the target handler sees the callsite of the thread that 
triggers the push.  Most use cases of MemoryHandler are records that are 
loggable but, get discarded and never published to the target handler (never 
formatted nor sent to some data sink).  So this is a real world use case of 
only openjdk classes.
 
The Peabody fix I proposed in 2007 was to unconditionally force the caller to 
be computed prior to adding the LogRecord to the internal data structure.  
Therefore all loggable records would pay the cost of inferring the caller.  
Current code is fast and broken (assuming target formatter is showing callsite) 
and the correct code will be slower.  If I were to redo that patch from Peabody 
I would think that PR review would bring to light a consistent view that 
StackWalker is fast enough at least the openjdk logging.  Effectively defining 
the minimum performance standard.  However, if it raises performance regression 
concerns perhaps there is some more work to be done improving StackWalker? :)

Jason



From: core-libs-dev  on behalf of Bernd 
Eckenfels 
Sent: Thursday, April 7, 2022 1:02 PM
To: core-libs-dev@openjdk.java.net
Subject: Re: fast way to infer caller

Some loggers do need to find the location of the log statement (class and line 
where the logger is used not where it is instantiated).

 for those (it makes loggers more useful) getting the call site is time 
critical even if they are not in tight performance critical loops.

But it actually does matter if/how the JVM optimizes such introspection.. if 
the JVM can inline (and maybe even constant intrinsic) the stalkwalker it would 
benefit such use cases just as well.

--
https://bernd.eckenfels.net

From: core-libs-dev  on behalf of Michael 
Kuhlmann 
Sent: Thursday, April 7, 2022 7:55:16 PM
To: core-libs-dev@openjdk.java.net 
Subject: Re: fast way to infer caller



On 4/7/22 19:27, Kasper Nielsen wrote:
>>
>> nope, see my previous mail to Ceki, the VM is cheating here if it can
>> inline the call to MethodHandles.lookup()
>>
>
> Does how the VM cheats really matter? The fact is that the code in the JDK
> can
> get the calling class and implement something like MethodHandles.lookup() so
> it takes ~3 ns. If you implemented something like a lookup class as a normal
> user your best bet would be StackWalker.GetCallingClass() and you would end
> up with something that is at least 2 magnitudes slower. That is probably not
> an issue for most use cases. But for some, it might be a bit of a steep
> cost.
>
> /Kasper

Hi Kasper,

sorry to jump in here as an uninvolved onlooker, but I can't resist.
I really don't see why this should matter. Getting the caller class is a
rare edge case that you just do in exceptional situations; most often
it's more for debugging or something.

What users really are interested in is high performance for standard
cases. Implementing a specific optimization into Hotspot to gain few
milliseconds is the least thing I expect from the JVM developers.

I also don't understand why someone should instantiate a logger during
performance critical procedures. In more than 20 years of Java
development, I've never seen the need to create a logger on the fly.
They are *always* assigned to final static variables, or at least to
predefined pools. Everything else would be just wrong: To instantiate a
logger, you have to fetch at least the log level definition from some
configuration source, and this can never be fast. At least not that
we're talking about nanoseconds here.

All logging implementations I know of (and all that make sense) are
highly optimized on log throughput; this can only be achieved by
preprocessing during initialization, why this is slow. But that doesn't
matter, because, as said, you should anyway create logger instances
beforehand.

Sorry for the rant, but I really don't see the use case here.

Re: fast way to infer caller

[Jason Mehrens]

Ceki,

Looks like the benchmark code is from https://github.com/qos-ch/slf4j/pull/271

Looks like the benchmark code is doing a collection so perhaps that is some of 
the performance hit?
Have you benchmarked java.util.LogRecord.getSourceClassName() to compare with 
your StackWalker benchmark?

https://github.com/openjdk/jdk/blob/master/src/java.logging/share/classes/java/util/logging/LogRecord.java#L754

Jason


From: core-libs-dev  on behalf of Ceki 
Gülcü 
Sent: Wednesday, April 6, 2022 4:26 PM
To: core-libs-dev
Subject: Re: fast way to infer caller


Hi Rémi,

Thank you for your answer.

According to some benchmarks on a i7-8565U Intel CPU (quite average
CPU), here are the costs of computing the caller class via different
methods:

using new Throwable().getStackTrace: 11 microseconds per call
using StackWalker API: 1.8 microseconds per call
using SecurityManager: 0.9 microseconds per call

While a six fold improvement (StackWalker compared to new Thorowable) is
nothing to sneeze at, the performance of StackWalker is not as good as
with a custom SecurityManager.

I have not said so explicitly but the aim here is to allow the user to
obtain a new logger by calling LoggerFactory.getLogger() with no
arguments with the returned logger named after the caller class.

Spending 1 or 2 microseconds for this call is OK if the logger is a
static field. However, if the logger is an instance field, then spending
2 microseconds per host object instance just to obtain a logger might
have a noticeable impact on performance. It follows that the performance
of LoggerFactory.getLogger() must be exceptionally good, assuming we
wish to avoid having the user accidentally shooting herself on the foot,
ergo the 100 nanosecond performance per call requirement.

Noting that invoking MethodHandles.lookup().lookupClass() seems very
fast (about 2 nanoseconds), I would be very interested if  new
lookup(int index) method were added to java.lang.invoke.MethodHandles
class, with index designating the desired index on the call stack.

Does the above make sense? How difficult would it be to add such a method?

--
Ceki Gülcü


On 4/6/2022 5:52 PM, Remi Forax wrote:
> - Original Message -
>> From: "Ceki Gülcü" 
>> To: "core-libs-dev" 
>> Sent: Wednesday, April 6, 2022 5:30:51 PM
>> Subject: fast way to infer caller
>
>> Hello,
>
> Hello,
>
>>
>> As you are probably aware, one of the important primitives used in
>> logging libraries is inferring the caller of a given logging statement.
>> The current common practice is to create a throwable and process its
>> stack trace. This is rather wasteful and rather slow. As an alternative,
>> I have tried using the StackWalker API to infer the caller but was
>> unsatisfied with the performance.
>>
>> MethodHandles.lookup().lookupClass() looks very promising except that
>> there is no way to specify the depth.
>>
>> I am looking for a method to obtain the Nth caller at a cost of around
>> 100 to 200 nanoseconds of CPU time.  Do you think the JDK could cater
>> for this use case?
>
> We have designed the StackWalker with that in mind
> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/StackWalker.html
>
> see the discussion on StackWalker.getCallerClass()
> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/lang/StackWalker.html#getCallerClass()
>

Re: OutputStreamWriter (not) flushing stateful Charsetencoder

[Jason Mehrens]

Here are the old details I can dig up when we ran into this on JavaMail:

https://bugs.openjdk.java.net/browse/JDK-6995537
https://github.com/javaee/javamail/commit/145d18c1738d3cf33b52bc005835980ff78ce4af

I recall digging through the code years ago and I don't recall if adding

w.write("");

Will trigger the encoder flush.  Not that is a great workaround either.

Jason


From: core-libs-dev  on behalf of Bernd 
Eckenfels 
Sent: Wednesday, November 10, 2021 8:12 AM
To: core-libs-dev
Subject: OutputStreamWriter (not) flushing stateful Charsetencoder

(I thought this was discussed  a while back on a OpenJDK mailing list, but I 
can’t find it. So apologies if this is a duplicate, but I might have seen it on 
Apache Commons-io instead - which fixed a similar issue on reader side)

The problem: I have code using a OutputStreamWriter with a customer defined 
charset name. this writer is flushed, and the code expects all pending bytes to 
be written. However when a stateful charset like cp930 is used, this is not the 
case. The final unshift byte for example is only written when the writer is 
closed. This is probably because it does not call end of data encode on the 
encoder in the flush().

The class does not clearly say or not say what is the correct behavior, however 
the flush() is formulated in a way that one could expect it should produce a 
complete stream.

So, is this a Bug in the implementation, if not should it be added to the 
documentation?

Here is a small JShell reproducer, you see the extra unshift byte (dec 15) only 
after the close:

var b = new byte[] { 0x31, (byte)0xef, (byte)0xbc, (byte)0x91 };
var s = new String(b, "UTF-8"); // „1２“ (1 is ascii, 2 is fw)
var bos = new ByteArrayOutputStream();
var w = new OutputStreamWriter(bos, "cp930"); // stateful ebcdic with Shift 
chars
w.write(s);
w.flush();
bos.toByteArray()
$8 ==> byte[4] { -15, 14, 66, -15 }
w.close();
bos.toByteArray()
$10 ==> byte[5] { -15, 14, 66, -15, 15 }

--
http://bernd.eckenfels.net

Re: Monitoring wrapped ThreadPoolExecutor returned from Executors

[Jason Mehrens]

Hi Doug,

What are your thoughts on promoting monitoring methods from TPE and or FJP to 
AbstractExecutorService?  The default implementations could just return -1.  An 
example precedent is OperatingSystemMXBean::getSystemLoadAverage.  The 
Executors.DelegatedExecutorService could then be modified to extend 
AbstractExecutorService and forward the new methods and the existing 
AES::taskFor calls when the wrapped Executor is also an 
AbstractExecutorService.  The return types of the Executors.newXXX would remain 
the same.

I suppose the tradeoff is that adding any new default method to ExecutorService 
and or new methods to AbstractExecutorService could break 3rd party code.

Jason


From: core-libs-dev  on behalf of Doug Lea 

Sent: Thursday, January 7, 2021 7:09 AM
To: core-libs-dev@openjdk.java.net
Subject: Re: Monitoring wrapped ThreadPoolExecutor returned from Executors

On 1/5/21 10:11 PM, Tommy Ludwig wrote:
> In the Micrometer project, we provide metrics instrumentation of 
> `ExectorService`s. For `ThreadPoolExecutor`s, we track the number of 
> completed tasks, active tasks, thread pool sizes, task queue size and 
> remaining capacity via methods from `ThreadPoolExecutor`. We are currently 
> using a brittle reflection hack[1] to do this for the wrapped 
> `ThreadPoolExecutor` returned from `Executors` methods 
> `newSingleThreadExecutor` and `newSingleThreadScheduledExecutor`. With the 
> introduction of JEP-396 in JDK 16, our reflection hack throws an 
> InaccessibleObjectException by default.
>
> I am not seeing a proper way to get at the methods we use for the metrics 
> (e.g. `ThreadPoolExecutor::getCompletedTaskCount`) in this case. Is there a 
> way that I am missing?

There's no guarantee that newSingleThreadExecutor returns a restricted
view of a ThreadPoolExecutor, so there can't be a guaranteed way of
accessing it,

But I'm sympathetic to the idea that under the current implementation
(which is unlikely to change anytime soon), the stats are available, and
should be available to monitoring tools. But none of the ways to do this
are very attractive: Creating a MonitorableExecutorService interface and
returning that? Making the internal view class public with a protected
getExecutor method?

Re: RFR: 8253459: Formatter treats index, width and precision > Integer.MAX_VALUE incorrectly [v4]

[Jason Mehrens]

Ian,

Should IllegalFormatArgumentIndexException.java provide an override of 
getMessage?  It appears that is done in the following:

https://github.com/openjdk/jdk/blob/9b07ef33b6e07afae1a8bfa034200eb3aab1f94f/src/java.base/share/classes/java/util/IllegalFormatWidthException.java
https://github.com/openjdk/jdk/blob/9b07ef33b6e07afae1a8bfa034200eb3aab1f94f/src/java.base/share/classes/java/util/IllegalFormatPrecisionException.java

Otherwise the index won't be rendered when toString or printStackTrace is 
invoked.

Jason


From: core-libs-dev  on behalf of Ian 
Graves 
Sent: Wednesday, October 14, 2020 1:39 PM
To: core-libs-dev@openjdk.java.net; i18n-...@openjdk.java.net
Subject: Re: RFR: 8253459: Formatter treats index, width and precision > 
Integer.MAX_VALUE incorrectly [v4]

> The `java.util.Formatter` format specifies support for field widths, argument 
> indexes, or precision lengths of a field
> that relate to the variadic arguments supplied to the formatter. These 
> numbers are specified by integers, sometimes
> negative. For argument index, it's specified in the documentation that the 
> highest allowed argument is limited by the
> largest possible index of an array (ie the largest possible variadic index), 
> but for the other two it's not defined.
> Moreover, what happens when a number field in a string is too large or too 
> small to be represented by a 32-bit integer
> type is not defined.  This fix adds documentation to specify what error 
> behavior occurs during these cases.
> Additionally it adds an additional exception type to throw when an invalid 
> argument index is observed.  A CSR will be
> required for this PR.

Ian Graves has updated the pull request incrementally with one additional 
commit since the last revision:

  Tweaking verbiage

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/516/files
  - new: https://git.openjdk.java.net/jdk/pull/516/files/391449a8..9b07ef33

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=516=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=516=02-03

  Stats: 18 lines in 2 files changed: 0 ins; 5 del; 13 mod
  Patch: https://git.openjdk.java.net/jdk/pull/516.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/516/head:pull/516

PR: https://git.openjdk.java.net/jdk/pull/516

Re: [PATCH] 8245694 : java.util.Properties.entrySet() does not override java.lang.Object methods

[Jason Mehrens]

Yu Li,

You should consider changing the equals implementation to include an identity 
self check.

public boolean equals(Object o) {
return this == o || entrySet.equals(o);
}

This is consistent with the collection wrapper classes in j.u.Collections.

Jason


From: core-libs-dev  on behalf of Lisa 
Li 
Sent: Saturday, May 30, 2020 9:28 AM
To: core-libs-dev@openjdk.java.net
Subject: [PATCH] 8245694 : java.util.Properties.entrySet() does not override 
java.lang.Object methods

Hi,

Please help review the fix for JDK-8245694
.  And this is my very
first patch submission. I know it's not perfect.



*BUG DESCRIPTION*:

JDK-8245694  :
java.util.Properties.entrySet()
does not override java.lang.Object methods since java 9.


*PROPOSED FIX*:

Add delegating overrides for equals(), hashCode(), and toString().


*PATCH*:

# HG changeset patch
# User yuli
# Date 1590841711 -28800
#  Sat May 30 20:28:31 2020 +0800
# Node ID 7dc946e5b5863291fa070bfdbdce48aefbe87b7b
# Parent  8e28aae5068069e853673148e4d3f44cb50350a7
8245694: java.util.Properties.entrySet() does not override Object methods
Summary: Add missing override methods
Contributed-by: Yu Li 

diff --git a/src/java.base/share/classes/java/util/Properties.java
b/src/java.base/share/classes/java/util/Properties.java
--- a/src/java.base/share/classes/java/util/Properties.java
+++ b/src/java.base/share/classes/java/util/Properties.java
@@ -1414,6 +1414,21 @@
 return entrySet.retainAll(c);
 }

+ @Override
+public boolean equals(Object o) {
+return entrySet.equals(o);
+}
+
+@Override
+public int hashCode() {
+return entrySet.hashCode();
+}
+
+@Override
+public String toString() {
+return entrySet.toString();
+}
+
 @Override
 public Iterator> iterator() {
 return entrySet.iterator();
diff --git a/test/jdk/java/util/Properties/PropertiesEntrySetTest.java
b/test/jdk/java/util/Properties/PropertiesEntrySetTest.java
new file mode 100644
--- /dev/null
+++ b/test/jdk/java/util/Properties/PropertiesEntrySetTest.java
@@ -0,0 +1,90 @@
+/*
+ * Copyright (c) 2018, 2019, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License
version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @summary tests the entrySet() method of Properties class
+ * @author  Yu Li
+ * @bug 8245694
+ */
+
+import java.util.Map;
+import java.util.Properties;
+import java.util.Set;
+
+public class PropertiesEntrySetTest {
+
+public static void main(String[] args) {
+
+testEntrySet();
+
+}
+
+private static void assertTrue(boolean value) {
+if (!value) {
+throw new RuntimeException("Failure in Properties testing.");
+}
+}
+
+private static void assertFalse(boolean value) {
+if (value) {
+throw new RuntimeException("Failure in Properties testing.");
+}
+}
+
+private static void testEntrySet() {
+Properties a = new Properties();
+Set> aEntrySet = a.entrySet();
+assertFalse(aEntrySet.equals(null));
+assertTrue(aEntrySet.equals(aEntrySet));
+
+Properties b = new Properties();
+Set> bEntrySet = b.entrySet();
+assertTrue(aEntrySet.equals(bEntrySet));
+assertTrue(aEntrySet.hashCode() == bEntrySet.hashCode());
+
+a.setProperty("p1", "1");
+assertFalse(aEntrySet.equals(bEntrySet));
+assertFalse(bEntrySet.equals(aEntrySet));
+assertFalse(aEntrySet.hashCode() == bEntrySet.hashCode());
+
+b.setProperty("p1", "1");
+assertTrue(aEntrySet.equals(bEntrySet));
+assertTrue(aEntrySet.hashCode() == bEntrySet.hashCode());
+
+Properties c = new Properties();
+c.setProperty("p1", "2");
+Set> cEntrySet = 

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

[Jason Mehrens]

Biasing towards ordering is covered in 
https://bugs.openjdk.java.net/browse/JDK-8181146 (read the markmail thread link 
in the ticket) and some of those changes worked there way into
http://hg.openjdk.java.net/jdk/jdk14/file/6c954123ee8d/src/java.base/share/classes/java/util/concurrent/ConcurrentSkipListMap.java#l1708
 and 
http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/main/java/util/concurrent/ConcurrentSkipListMap.java?r1=1.176=1.177

I think there is some merit in even biasing toward ordering for non-sorted 
sets/maps to avoid expensive contains calls.  Any change we apply to equals we 
can usually apply a similar idea to containsAll.

I'm not sure about computing the hash code during equals.  It is not ideal but 
seems to solve the case you raised. I doubt it passes the compatibility bar for 
behavior or performance.

Jason


From: Alan Snyder 
Sent: Wednesday, May 20, 2020 1:53 PM
To: Jason Mehrens
Cc: core-libs-dev
Subject: Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are 
surprisingly dependent on relative sizes

Do you believe that Set.equals() should behave this way on SortedSets?

On May 20, 2020, at 11:23 AM, Jason Mehrens 
mailto:jason_mehr...@hotmail.com>> wrote:

public class SetEquals {

   public static void main(String[] args) {
   Comparator cc = String.CASE_INSENSITIVE_ORDER;
   Set s1 = new TreeSet<>(cc);
   Set s2 = new TreeSet<>(cc);
   s1.add("hello");
   s2.add("Hello");
   System.out.println(equals(s1, s2));
   System.out.println(equals(s2, s1));
   System.out.println(s1.hashCode() == s2.hashCode());
   }

   private static boolean equals(Set s1, Set s2) {
   if (s2 == null) {
   return false;
   }

   if (s1 == s2) {
   return true;
   }

   int s1h = 0;
   int s2h = 0;
   Iterator e1 = s1.iterator();
   Iterator e2 = s2.iterator();

   if (s1 instanceof SortedSet && s2 instanceof SortedSet
   && isSameOrder((SortedSet) s1, (SortedSet) s2)) {
   //TODO: raw type warnings.
   Comparator cmp = ((SortedSet) s1).comparator();
   if (cmp == null) {
   cmp = Comparator.naturalOrder();
   }
   while (e1.hasNext() && e2.hasNext()) {
   Object o1 = e1.next();
   Object o2 = e2.next();

   try {
   if (cmp.compare(o1, o2) != 0
   && (!s1.contains(o2) || !s2.contains(o1))) {
   return false;
   }
   } catch (ClassCastException cce) {
   return false;
   }

   s1h += Objects.hashCode(o1);
   s2h += Objects.hashCode(o2);

   //Iteration order should be the same.
   if (s1h != s2h) {
   return false;
   }
   }
   } else {
   while (e1.hasNext() && e2.hasNext()) {
   Object o1 = e1.next();
   Object o2 = e2.next();

   try {
   if (!Objects.equals(o1, o2)
   && (!s1.contains(o2) || !s2.contains(o1))) {
   return false;
   }
   } catch (ClassCastException cce) {
   return false;
   }

   s1h += Objects.hashCode(o1);
   s2h += Objects.hashCode(o2);
   }
   }
   return s1h == s2h && !e1.hasNext() && !e2.hasNext();
   }

   private static boolean isSameOrder(SortedSet s1, SortedSet s2) {
   //null matches natural ordering or unwrap to find a singleton.
   //Avoid calling equals on comparator.
   return Collections.reverseOrder(s1.comparator())
   == Collections.reverseOrder(s2.comparator());
   }
}

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

[Jason Mehrens]

Alan,

This contains that and that contains this at least avoids calling size which 
could be expensive or ephemeral at the AbstractSet level.  Using the iterators 
you can at least do a final check to see there are no more elements to walk.   
Biasing towards ordering could pay off for things like naturally ordered 
SortedSet to SortedSet, LinkedHashSet, or CopyOnWriteArraySet.

Computing the hash during equals is about the best I can come up with:  

public class SetEquals {

public static void main(String[] args) {
Comparator cc = String.CASE_INSENSITIVE_ORDER;
Set s1 = new TreeSet<>(cc);
Set s2 = new TreeSet<>(cc);
s1.add("hello");
s2.add("Hello");
System.out.println(equals(s1, s2));
System.out.println(equals(s2, s1));
System.out.println(s1.hashCode() == s2.hashCode());
}

private static boolean equals(Set s1, Set s2) {
if (s2 == null) {
return false;
}

if (s1 == s2) {
return true;
}

int s1h = 0;
int s2h = 0;
Iterator e1 = s1.iterator();
Iterator e2 = s2.iterator();

if (s1 instanceof SortedSet && s2 instanceof SortedSet
&& isSameOrder((SortedSet) s1, (SortedSet) s2)) {
//TODO: raw type warnings.
Comparator cmp = ((SortedSet) s1).comparator();
if (cmp == null) {
cmp = Comparator.naturalOrder();
}
while (e1.hasNext() && e2.hasNext()) {
Object o1 = e1.next();
Object o2 = e2.next();

try {
if (cmp.compare(o1, o2) != 0
&& (!s1.contains(o2) || !s2.contains(o1))) {
return false;
}
} catch (ClassCastException cce) {
return false;
}

s1h += Objects.hashCode(o1);
s2h += Objects.hashCode(o2);

//Iteration order should be the same.
if (s1h != s2h) {
return false;
}
}
} else {
while (e1.hasNext() && e2.hasNext()) {
Object o1 = e1.next();
Object o2 = e2.next();

try {
if (!Objects.equals(o1, o2)
&& (!s1.contains(o2) || !s2.contains(o1))) {
return false;
}
} catch (ClassCastException cce) {
return false;
}

s1h += Objects.hashCode(o1);
s2h += Objects.hashCode(o2);
}
}
return s1h == s2h && !e1.hasNext() && !e2.hasNext();
}

private static boolean isSameOrder(SortedSet s1, SortedSet s2) {
//null matches natural ordering or unwrap to find a singleton.
//Avoid calling equals on comparator.
return Collections.reverseOrder(s1.comparator())
== Collections.reverseOrder(s2.comparator());
}
}

Jason
________
From: Alan Snyder 
Sent: Thursday, May 14, 2020 8:39 AM
To: Jason Mehrens
Cc: core-libs-dev
Subject: Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are 
surprisingly dependent on relative sizes

> HashSet/TreeSet could do what ConcurrentHashMap/ConcurrentSkipListSet do by 
> using the "this contains that and that contains this" logic.

Yes, at the cost of yet another performance regression.

But how about this problem:

Comparator cc = String.CASE_INSENSITIVE_ORDER;
Set s1 = new TreeSet<>(cc);
Set s2 = new TreeSet<>(cc);
s1.add("hello");
s2.add("Hello");
s1.equals(s2) -> true
s2.equals(s1) -> true
s1.hashCode() == s2.hashCode() -> false

Re: RFR: 8215401: Add isEmpty default method to CharSequence

[Jason Mehrens]

Claes,

I would think CharBuffer would require some testing in your test cases too.  
Also it looks like some of the CharSequence methods in CharBuffer are declared 
final.  Not sure what is appropriate here as far as CharBuffer::isEmpty 
modifiers are concerned.

Jason


From: core-libs-dev  on behalf of Claes 
Redestad 
Sent: Tuesday, May 19, 2020 5:37 AM
To: Roger Riggs; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8215401: Add isEmpty default method to CharSequence

Hi Roger,

sure, added Emptiness.java with a few sanity tests.

/Claes

On 2020-05-19 00:29, Roger Riggs wrote:
> Hi Claes,
>
> Though it does not look like it could be any simpler, there should
> probably be a test.
> Checking consistency with the existing implementations of
> CharSequence.length()
> and a custom subtype.
>
> Thanks, Roger
>
>
> On 5/18/20 4:48 PM, Claes Redestad wrote:
>> Hi,
>>
>> let's add an isEmpty default method to CharSequence!
>>
>> Webrev: http://cr.openjdk.java.net/~redestad/8215401/open.00/
>> RFE:https://bugs.openjdk.java.net/browse/JDK-8215401
>> CSR:https://bugs.openjdk.java.net/browse/JDK-8215402
>>
>> Testing: tier1-3
>>
>> /Claes
>

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

[Jason Mehrens]

I think you have it covered. Thanks for the feedback it for sure helped me 
understand the approach.

So on a lighthearted note I did figure out a way (tongue in cheek) to test if 
Collection::contains is expensive. You could even figure out if the membership 
semantics of each collection are the same without even creating new APIs.  
Anyway, don't take this example seriously and enjoy!
  
   @Deprecated
 public static boolean removeAllTester(Collection dis, Collection c) {
Objects.requireNonNull(c);
boolean modified = false;

class Ref {
int count;
Ref() {
}

public boolean equals(Object o) {
count++;
return super.equals(o);
}

public int hashCode() {
count++;
return super.hashCode();
}
}

boolean expensiveContains;
try {
Ref test = new Ref();
c.contains(test);
//Identity == zero and greater than 10 is a poor bin length
//Obviously this fails if the collection::contains performs 
'e.equals(o)'
expensiveContains = test.count >= 10;
} catch (RuntimeException usesCompare) {
expensiveContains = false;
}

if (expensiveContains) {
for (Object e : c)
modified |= dis.remove(e);
} else {
for (Iterator i = dis.iterator(); i.hasNext(); ) {
if (c.contains(i.next())) {
i.remove();
modified = true;
}
}
}
return modified;
}

public static void main(String[] args) {
int sourceSize = 30;
int removalsSize =  30;

Set source = new HashSet();
Collection removals = new ArrayList();

for (int i = 0; i < sourceSize; i++)
source.add(i);
for (int i = 1; i <= removalsSize; i++)
   removals.add(-i);

long start = System.currentTimeMillis();
boolean modified;

//modified = source.removeAll(removals);
modified = removeAllTester(source, removals);

//removals.forEach(e -> source.remove(e));
//source.removeIf(e -> removals.contains(e));
long end = System.currentTimeMillis();
System.out.println("Time taken: " + (end - start) + "ms " + modified);
}


Jason

From: Stuart Marks 
Sent: Tuesday, May 12, 2020 3:34 PM
To: Jason Mehrens
Cc: core-libs-dev
Subject: Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are 
surprisingly dependent on relative sizes



On 5/8/20 6:46 PM, Jason Mehrens wrote:
> I assume Deque/Queue would suffer the same issue as List.  It would be nice 
> to some how ensure ArrayDeque.contains is not called as far as the heuristic 
> goes.  Looks like PriorityQueue uses equals for contains but that is not to 
> say there are not other queue implementations in the wild that do something 
> different.

The difficulty here is how far to take the heuristic. The goal here is not to
guarantee that behavior can never be O(M*N) but to catch what seem to be the
most common cases. I think the most common case is where the argument is a List,
but there are lots of possibilities that we'd inevitably miss.

> The more I think about it in general, it seems like your task would be easier 
> if you could declare:
> 1. AbstractCollection.removeAll should assume this.contains is O(N) and 
> iterate over this and check contains on arg.
> 2. AbstractSet.removeAll should iterate over argument and assume that 
> this.contains/remove is at least O(log (N)) and assume argument.contains is 
> O(N).
> 3. Concrete implementations of removeAll know the cost of their contains 
> method.  If it is known to be O(N) then walk this otherwise walk the arg.
> 4. Include an example in removeAll that says if membership semantics differ 
> between sets use: source.removeIf(e -> removals.contains(e)); or 
> removals.forEach(e -> source.remove(e)); instead.  If they don't differ then 
> use removeAll.

This sort of makes sense from a performance standpoint, but it doesn't address
the semantic issues I raised in my reply the other day to Jens Lideström.
Specifically, we don't want Set.removeAll or AbstractSet.removeAll to disagree
with Collection.removeAll and AbstractCollection.removeAll. We also want
removeAll() to be the complement of retainAll().

> Given this has been around since JDK 1.3 would it be safe to assume that code 
> that is mixing collections with different membership semantics is already 
> working around this issue and therefore the risk is a bit lower in changing 
> the spec?  Lots of concrete implementations al

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

[Jason Mehrens]

HashSet/TreeSet could do what ConcurrentHashMap/ConcurrentSkipListSet do by 
using the "this contains that and that contains this" logic.

  Comparator cc = String.CASE_INSENSITIVE_ORDER;
Set s1 = new ConcurrentHashMap().keySet("");
Set s2 = new ConcurrentSkipListSet<>(cc);
s1.add("hello");
s2.add("Hello");
System.out.println(s1.equals(s2));
System.out.println(s2.equals(s1));
System.out.println(s1.hashCode() == s2.hashCode());
===
false
false
false


From: core-libs-dev  on behalf of Alan 
Snyder 
Sent: Tuesday, May 12, 2020 11:27 AM
To: Stuart Marks
Cc: core-libs-dev
Subject: Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are 
surprisingly dependent on relative sizes



> On May 8, 2020, at 1:49 PM, Stuart Marks  wrote:
>
> The containsAll() and equals() methods both use the membership contract of 
> the receiver, not the argument. Unfortunately, the equals() specification 
> says,
>
>Returns true if the specified object is also a set, the two sets have the
>same size, and every member of the specified set is contained in this set
>(or equivalently, every member of this set is contained in the specified
>set).
>
> As should be clear from this discussion, the "equivalently" clause is 
> incorrect -- another spec bug.

Changing Set.equals() in this way would make Set inconsistent with Object.
Do you really think that is a good idea?




Comparator cc = (a, b) -> a.compareToIgnoreCase(b);

Set s1 = new HashSet<>();

Set s2 = new TreeSet<>(cc);

s1.add("hello");

s2.add("Hello");

s1.equals(s2) -> false

s2.equals(s1) -> true

s1.hashCode() == s2.hashCode() -> false

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

[Jason Mehrens]

Stuart,

I assume Deque/Queue would suffer the same issue as List.  It would be nice to 
some how ensure ArrayDeque.contains is not called as far as the heuristic goes. 
 Looks like PriorityQueue uses equals for contains but that is not to say there 
are not other queue implementations in the wild that do something different.

The more I think about it in general, it seems like your task would be easier 
if you could declare:
1. AbstractCollection.removeAll should assume this.contains is O(N) and iterate 
over this and check contains on arg.
2. AbstractSet.removeAll should iterate over argument and assume that 
this.contains/remove is at least O(log (N)) and assume argument.contains is 
O(N).
3. Concrete implementations of removeAll know the cost of their contains 
method.  If it is known to be O(N) then walk this otherwise walk the arg.
4. Include an example in removeAll that says if membership semantics differ 
between sets use: source.removeIf(e -> removals.contains(e)); or 
removals.forEach(e -> source.remove(e)); instead.  If they don't differ then 
use removeAll.

Given this has been around since JDK 1.3 would it be safe to assume that code 
that is mixing collections with different membership semantics is already 
working around this issue and therefore the risk is a bit lower in changing the 
spec?  Lots of concrete implementations already override removeAll anyway.

Jason







From: core-libs-dev  on behalf of 
Stuart Marks 
Sent: Monday, May 4, 2020 7:25 PM
To: Jason Mehrens
Cc: core-libs-dev
Subject: Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are 
surprisingly dependent on relative sizes



On 5/1/20 10:41 PM, Jason Mehrens wrote:
> 1. I assume you are using "c instanceof List" instead of "!(c instanceof 
> Set)" to correctly handle IdentitityHashMap.values()?  The instanceof List 
> seems like safe choice but it is too bad we can still fool that check by 
> wrapping List as an unmodifiableCollection.  If 
> splitIterator().characteristics() could tell us that the collection used 
> identity comparisons I think we would be able to determine if it was safe to 
> swap the ordering in the general case as we could check for IDENTITY, SORTED, 
> and comparator equality.

I'm using "instance List", not for the reason of IdentityHashMap.values()
specifically (though that's a good example), but mainly to try to be minimal.
While I think getting the semantics right takes priority, the change does impact
performance, so I want to reintroduce the performance heuristic in the safest
manner possible. Checking for !Set seems dangerous, as there might be any number
of Collections out there that aren't Sets and that aren't consistent with
equals. Checking for instanceof List seemed like the safest and most minimal
thing to do.

In fact, I'm not even sure how safe List is! It's certainly possible for someone
to have a List that isn't consistent with equals. Such a thing might violate the
List contract, but that hasn't stopped people from implementing such things
(outside the JDK).

I also toyed around with various additional tests for when it would be
profitable to switch iteration to the smaller collection. This could be applied
to a variety of consistent-with-equals set implementations in the JDK. The
benefits of swapping the iteration is more modest in these cases compared to
List, however. Avoiding repeated List.contains() helps avoid quasi-quadratic
(O(M*N)) performance. Swapping iteration order of sets gets us only the smaller
of O(M) vs O(N), which is still linear.

Also, as you noted, this heuristic is easily defeated by things like the
collection wrappers.

> 2. Should code applied to HashSet.removeAll also be applied to 
> HashMap.keySet().removeAll and HashMap.entrySet().removeAll?  
> Collections::newSetFromMap will end up having different behavior if it is not 
> consistently applied.

I think the *results* will be consistent, but the *performance* might not be
consistent.

But these cases could result in pathological performance if removeAll(list) is
called, so I'm a bit concerned about them. If we agree that "instanceof List" is
a reasonable heuristic, then I don't have any objection in principle to adding
it to these locations as well. But I want to be careful about sprinkling ad hoc
policies like this around the JDK.

I note that the erroneous size-based optimization was copied into, and therefore
needs to be removed from ConcurrentSkipListSet (JDK-8218945) and the set views
of CHM (JDK-8219069). I'd more concerned about getting these cleaned up in the
short term.

> 3. Not to derail this thread but do think we need a new JIRA ticket to 
> address Collections.disjoint?  Seems like it has similar sins of calling size 
> and using "!(c2 instanceof Set)" but I don't want to muddy the waters by 
> covering any solutions to that meth

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

[Jason Mehrens]

1. I assume you are using "c instanceof List" instead of "!(c instanceof Set)" 
to correctly handle IdentitityHashMap.values()?  The instanceof List seems like 
safe choice but it is too bad we can still fool that check by wrapping List as 
an unmodifiableCollection.  If splitIterator().characteristics() could tell us 
that the collection used identity comparisons I think we would be able to 
determine if it was safe to swap the ordering in the general case as we could 
check for IDENTITY, SORTED, and comparator equality.
2. Should code applied to HashSet.removeAll also be applied to 
HashMap.keySet().removeAll and HashMap.entrySet().removeAll?  
Collections::newSetFromMap will end up having different behavior if it is not 
consistently applied.
3. Not to derail this thread but do think we need a new JIRA ticket to address 
Collections.disjoint?  Seems like it has similar sins of calling size and using 
"!(c2 instanceof Set)" but I don't want to muddy the waters by covering any 
solutions to that method in this thread.

Jason



From: core-libs-dev  on behalf of 
Stuart Marks 
Sent: Friday, May 1, 2020 3:01 PM
To: core-libs-dev
Subject: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are 
surprisingly dependent on relative sizes

Hi all,

I'm taking another swing at this one. I've taken a minimal approach compared to
my previous proposals.

Briefly, AbstractSet.removeAll switches from iterating this collection and
calling contains() on its argument, to iterating the argument and calling
this.contains(), if the argument collection is smaller than this collection.
This attempt at optimization is incorrect, because this collection and the
argument collection might have different semantics for contains().

There is a confounding performance problem, which is that if the argument is a
List, contains() is generally linear, which can result in pathologically slow
(O(m*n)) performance. Because of the iteration-switching above, this performance
problem can appear and disappear based on the relative sizes, which is 
startling.

The fix for the semantic problem is simply to remove the attempted optimization
from AbstractSet. This comports with the specification of Collection.removeAll
and Set.removeAll, which imply that contains() is called on the argument
collection. This allows sets to inherit the implementation in
AbstractCollection.removeAll. In addition, this ensures that removeAll is now
always the complement of retainAll (as it should be), which it sometimes was not
when the optimization was in place.

IdentityHashMap's keySet and entrySet views were broken by this optimization, so
they had to override removeAll with copies of the implementation from
AbstractCollection. Since they can now inherit from AbstractCollection, these
overrides have been removed.

This leaves a potential performance problem with removeAll when the argument is
a List. To mitigate this, HashSet.removeAll switches to iterating the argument
if the argument is a List. This is safe, as both HashSet and List use the same
semantics for contains() (at least, as far as I know).

I've opted not to pursue size-based optimizations at this time, since they
provide only incremental benefit, whereas the pathological performance problem
mentioned above is the primary issue. Also, it's actually quite difficult to
determine when it's safe to switch iteration.

Finally, I've added performance notes to the specifications of containsAll(),
removeAll(), and retainAll(), warning about potential performance problems that
can occur with repeated calls to contains() made by these methods.

Bug:

 https://bugs.openjdk.java.net/browse/JDK-6394757

Webrev:

 http://cr.openjdk.java.net/~smarks/reviews/6394757/webrev.2/

Previous discussions:

 http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-July/007125.html

 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/058140.html

 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-February/058378.html

 http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-May/060007.html

 http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-May/060147.html

Thanks,

s'marks

Re: Collections.synchronizedXXX() and internal mutex (aka SyncRoot)

[Jason Mehrens]

Background on this can be found here: 
https://bugs.openjdk.java.net/browse/JDK-4335520

Jason


From: core-libs-dev  on behalf of 
dmytro sheyko 
Sent: Wednesday, April 29, 2020 2:58 AM
To: core-libs-dev
Subject: Collections.synchronizedXXX() and internal mutex (aka SyncRoot)

Hello,

Have you ever discussed to make field mutex in synchronized collections
accessible?

Javadoc for Collections#synchronizedSortedSet suggest to iterate collection
this way:

  SortedSet s = Collections.synchronizedSortedSet(new TreeSet());
  SortedSet s2 = s.headSet(foo);
  ...
  synchronized (s) {  // Note: s, not s2!!!
  Iterator i = s2.iterator(); // Must be in the synchronized block
  while (i.hasNext())
  foo(i.next());
  }

I.e. in order to iterate subset, we also need a reference to the whole set,
which is not really convenient. How about to make it possible to write:

  SortedSet s2 = s.headSet(foo);
  ...
  synchronized (Collections.getSyncRoot(s2)) {  // Note:
Collections.getSyncRoot(s2)
  Iterator i = s2.iterator(); // Must be in the synchronized block
  while (i.hasNext())
  foo(i.next());
  }

Also I think it would be convenient to let to provide custom sync root when
synchronized collection is created.
E.g.

  Object customSyncRoot = new Object();
  SortedSet s = Collections.synchronizedSortedSet(new TreeSet(),
customSyncRoot);

What do you think about this?

Regards,
Dmytro

Re: LinkedHashMap/LinkedHashSet enhancement: OrderedMap/OrderedSet

[Jason Mehrens]

Looks like It is intentional that unmodifiable queues are not present.  See: 
https://bugs.openjdk.java.net/browse/JDK-5030930.  The same logic would have 
been used for when Deque was added in the following release.

Jason


From: core-libs-dev  on behalf of 
Stuart Marks 
Sent: Tuesday, April 28, 2020 6:57 PM
To: Paul Sandoz
Cc: core-libs-dev
Subject: Re: LinkedHashMap/LinkedHashSet enhancement: OrderedMap/OrderedSet

Hi Paul,

I too hesitate about adding Ordered* interfaces. As I said previously, I don't
think they're very useful, and they do seem rather Queue- or Deque-like.

Indeed, Tagir was musing the other day on Twitter about a Deque view of
LinkedHashSet and possibly LinkedHashMap. I think that might be a more
worthwhile direction to pursue. Providing Deque views is more in keeping with
the current collections idioms than making LHS/LHM implement Deque directly.
This is rather like Map providing entry and key Set views and values Collection
views.

Note that these "view collections" provide live views of the backing collection
and that they aren't necessarily read-only. They support iteration in various
forms (e.g., streaming), and they can also permit removal (but not addition).
This is a bit odd but it can come in handy.

I'm thinking that the form of Iterable that supports access to first/last
elements, and iteration in either order, is in fact Deque itself. Deque already
supports descendingIterator(), which is a useful mechanism, but it's somewhat
inconvenient: you can't use it with enhanced-for, stream, or toArray. It would
be good to have a reversed-view Deque that flips the order of all the
operations. That avoids having to provide descendingStream(),
descendingToArray(), etc., and it adapts to anything that consumes an Iterable
or Collection.

(It occurs to me that we're missing a Collections.unmodifiableDeque wrapper.)

The LHS/LHM views, and various Deque wrappers, should all be pretty simple, with
the typical bunch of one-liner methods. (With the possible exception of
toArray(T[]).) Whether they are sufficiently useful, though, is still an open
question.

s'marks

On 4/28/20 10:59 AM, Paul Sandoz wrote:
> Hi Tagir,
>
> I am hesitant to add new interfaces specific to Set/Map for a non-indexed 
> encounter order.
>
> The interface OrderedSet is tantalizingly queue-like, in fact rather close to 
> the read-only part of Deque.  Did you consider the possibility of 
> LinkedHashSet implementing Deque?
>
> I have not thought about this too deeply, but since LinkedHashMap is 
> implemented as a doubly linked list it might be possible (as LinkedList 
> implements Deque).
>
> If so we could also add the method:
>
>LinkedHashSet> LinkedHashMap.entryLinkedSet()
>
> and thereby avoid the addition of any new interfaces, although it might 
> require some refinement to the specification of Deque; a new notion of an 
> unmodifiable queue, which I admit is a little odd, and as a result is 
> probably not be a good idea.
>
> More generally I think the core abstraction is really a new form Iterable 
> supporting access to first + last elements and reverse iteration (and 
> possibly iteration starting from a containing element).
>
> Paul.
>
>> On Apr 25, 2020, at 9:51 AM, Tagir Valeev  wrote:
>>
>> Hello!
>>
>> To me, it was always a pity that the internal structure of
>> LinkedHashMap allows doing more things easily, yet this functionality
>> is not exposed to the external clients, so if somebody really needs
>> such things they will need to waste memory/CPU to emulate them. The
>> functionality I'm speaking about includes:
>> - getting the last (i.e. most recently added/accessed) entry
>> - getting the descending iterator going from newest to oldest entries
>> - getting the iterator that starts at given existing entry
>>
>> It's easy to do such things with TreeMap and other NavigableMap
>> implementations. However, LinkedHashMap provides no additional visible
>> methods in addition to the Map interface (well, except
>> removeEldestEntry()). The same considerations apply to LinkedHashSet.
>>
>> So we can provide new interfaces j.u.OrderedMap and OrderedSet that
>> provide the additional functionality and make
>> LinkedHashMap/LinkedHashSet implement them. Aside from new
>> functionality, the interfaces will carry additional semantics: their
>> iteration order is always predictable and their spliterators return
>> DISTINCT|ORDERED characteristics, so some clients may require
>> OrderedMap/Set just to assert that they rely on predictable iteration
>> order. We can even make existing NavigableMap/Set extend
>> OrderedMap/Set because NavigableMap/Set are ordered. Some of
>> NavigableMap/Set methods will naturally fit the OrderedMap/Set
>> interfaces.
>>
>> I think I can devote my spare time to write the spec, implementation,
>> and tests for this enhancement, participate in the API design
>> discussions and do any other job necessary to make this improvement
>> 

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Jason Mehrens]

Would it work to fix this by making DeleteOnExitHook::runHooks deal with 
dependencies?
1. Remove If deleted, or not directory which also takes care of not exists.
2. Sort remaining files by deepest child files/directories first.
3. Run delete again on the list.

Otherwise files need to be processed in reverse order before directories and 
directories need to be processed children first up to the root.

Jason


From: core-libs-dev  on behalf of Ivan 
Gerasimov 
Sent: Wednesday, July 10, 2019 8:51 PM
To: Brian Burkhalter; core-libs-dev
Subject: Re: 8193072: File.delete() should remove its path from 
DeleteOnExitHook.files


On 7/10/19 5:17 PM, Brian Burkhalter wrote:
> I incorporated Peter’s version, adding the security check in 
> cancelDeleteOnExit(), tweaking its verbiage along with that of 
> deleteOnExit(), and modified the test DeleteOnExit to verify the new method. 
> The updated version is here:
>
> http://cr.openjdk.java.net/~bpb/8193072/webrev.03/ 
> 
There is possibility of a race here in a scenario like this:

File dir = new File("dir");
File file = new File("dir/file");

-- thread 1 --
dir.deleteOnExit();
file.deleteOnExit();
///
dir.cancelDeleteOnExit();
  thread 2 intervenes
file.cancelDeleteOnExit();
-- end --

-- thread 2 --
dir.deleteOnExit();
file.deleteOnExit();
-- end --

The result will be that the order of the registered files will change,
and JVM will try to delete dir first (this will fail as it is not empty).

Of course it could be avoided, if cancellation were done in reverse
order, though it's not immediately obvious from the documentation.

With kind regards,
Ivan
> Thanks,
>
> Brian
>
>> On Jul 10, 2019, at 11:17 AM, Brian Burkhalter  
>> wrote:
>>
>> On Jul 10, 2019, at 5:36 AM, Peter Levart  wrote:
>>> There are various interleavings of threads that could cause the file to be 
>>> left undeleted when VM exits.
>>>
>>> To cover concurrent scenarios such as above, the code could use reference 
>>> counting. Like in the following patch:
>>>
>>> http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.01/
>>>  
>>> 
>> This looks good to me modulo adding this
>> SecurityManager security = System.getSecurityManager();
>> if (security != null) {
>> security.checkDelete(path);
>> }
>> to cancelDeleteOnExit() as suggested below.
>>
>>> On Jul 10, 2019, at 7:51 AM, Sean Mullan  wrote:
>>>
>>> On 7/9/19 7:40 PM, Brian Burkhalter wrote:
 I don’t know. On the one hand this does not take an action like reading, 
 writing, or deleting, but on the other it could end up causing files to be 
 left lying around after VM termination which were expected to be deleted. 
 I suppose that could be considered to be some sort of security issue.
>>> Yes I think so.
>>>
>>> I would probably just use the same permission 
>>> (FilePermission(file,"delete")). If you have been granted permission to 
>>> delete a file, then you should have permission to cancel that deletion as 
>>> well.
>> That’s a  good idea.

--
With kind regards,
Ivan Gerasimov

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Jason Mehrens]

Would the SecurityManager need to for permissions (checkWrite or some new 
permission) before cancelDeleteOnExit() is allowed?

Jason


From: core-libs-dev  on behalf of Brian 
Burkhalter 
Sent: Tuesday, July 9, 2019 1:08 PM
To: core-libs-dev
Subject: Re: 8193072: File.delete() should remove its path from 
DeleteOnExitHook.files


> On Jul 9, 2019, at 8:31 AM, Brian Burkhalter  
> wrote:
>
>> Since deleteOnExit() is an deliberate act, perhaps there should be a 
>> corresponding withdrawDeleteOnExit() that reverses it so that it is 
>> intentional and not a side-effect of other File methods.
>
> I think this is a better idea. Perhaps “cancelDeleteOnExit()”.

If we want to go this route, here is one possibility:

http://cr.openjdk.java.net/~bpb/8193072/webrev.02/

Of course a CSR would be called for if this is agreed upon.

Thanks,

Brian

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Jason Mehrens]

Brian,

Just a note, one issue I see with webrev.01 is that JDK-7092892 is not 
resolved.  On one hand more calls to DeleteOnExitHook should trigger class init 
sooner avoiding the issue.  On the other it seems this could be more methods 
that could fail by throwing ExceptionInInitializerError that wouldn't have 
before the change.  I would think that you would want to mark JDK-7092892 as 
blocker of JDK-8193072 before you go this route.

Jason


From: core-libs-dev  on behalf of Brian 
Burkhalter 
Sent: Tuesday, July 9, 2019 10:07 AM
To: Roger Riggs
Cc: core-libs-dev@openjdk.java.net
Subject: Re: 8193072: File.delete() should remove its path from 
DeleteOnExitHook.files

Hi Roger,

You might be correct but I wrote up a different version at the end of 
yesterday. Not sure it is right but I might as well post it:

http://cr.openjdk.java.net/~bpb/8193072/webrev.01/

Thanks,

Brian

> On Jul 9, 2019, at 7:34 AM, Roger Riggs  wrote:
>
> The sequence described is the specified behavior of the API, whether it is a 
> developer mistake or not is unknowable but it would be a compatibility issue 
> to change it. The filename is the key and there is no way to determine if it 
> is the original file or a replacement. deleteOnExit Wins!

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Jason Mehrens]

Brian,

Previously File.delete wouldn't throw IllegalStateException and with this patch 
it looks like that is possible (and not desirable).  I would think that this 
change could the break java.util.logging.FileHandler because Handler.close runs 
in a shutdown hook.

Jason


From: core-libs-dev  on behalf of Brian 
Burkhalter 
Sent: Monday, July 8, 2019 3:11 PM
To: core-libs-dev
Subject: 8193072: File.delete() should remove its path from 
DeleteOnExitHook.files

https://bugs.openjdk.java.net/browse/JDK-8193072 


There does appear to be a memory leak of sorts if one does something like

—

File[] files;
for (int i = 0; i < largeNumber; i++) {
files[i] = File.createTempFile(“blah”, null);
files[i].deleteOnExit();
}

// do something

for (int i = 0; i < largeNumber; i++) {
files[i].delete();
}

// do something else before shutdown

—

The LinkedHashSet in DeleteOnExitHook will contain at least largeNumber Files 
until the VM shuts down even though the files were deleted.

The potential change is included below. The additional call to 
DeleteOnExitHook.remove() in File.delete() does not appear to have a measurable 
performance impact, at least trivially and in isolation.

Thanks,

Brian

--- a/src/java.base/share/classes/java/io/DeleteOnExitHook.java
+++ b/src/java.base/share/classes/java/io/DeleteOnExitHook.java
@@ -64,6 +64,15 @@
 files.add(file);
 }

+static synchronized void remove(String file) {
+if(files == null) {
+// DeleteOnExitHook is running. Too late to remove a file
+throw new IllegalStateException("Shutdown in progress");
+}
+
+files.remove(file);
+}
+
 static void runHooks() {
 LinkedHashSet theFiles;

--- a/src/java.base/share/classes/java/io/File.java
+++ b/src/java.base/share/classes/java/io/File.java
@@ -1050,6 +1050,7 @@
 if (isInvalid()) {
 return false;
 }
+DeleteOnExitHook.remove(path);
 return fs.delete(this);
 }

Re: [PATCH] 4511638: Double.toString(double) sometimes produces incorrect results

[Jason Mehrens]

Maybe rename the catch variable from 'cause' or 'ignored' to 'unreachable' and 
then wrap java.io.IOException in java.io.IOError?


From: core-libs-dev  on behalf of 
Raffaello Giulietti 
Sent: Thursday, April 18, 2019 3:37 PM
To: Brian Burkhalter; core-libs-dev
Subject: Re: [PATCH] 4511638: Double.toString(double) sometimes produces 
incorrect results

Hi,

On 18.04.19 21:29, Brian Burkhalter wrote:
>
>> On Apr 18, 2019, at 11:44 AM, Raffaello Giulietti
>> > > wrote:
>>
>> here's another revision of the patch. Its purpose is to overcome the
>> test failure observed in [1]. To this end, the patch adds
>>
>> FloatToDecimal.appendTo(float, Appendable) and
>> DoubleToDecimal.appendTo(double, Appendable)
>>
>> static methods to help AbstractStringBuilder in using the corrected
>> algorithm implemented in
>>
>> FloatToDecimal.toString(float) and
>> DoubleToDecimal.toString(double), respectively.
>>
>> The implementation has been jmh tested to make sure there are no
>> performance regressions.
>
> Thanks, Raffaello.
>
>> As there are now only less than two months left before Rampdown 1 for
>> OpenJDK 13, I beg anybody interested in reviewing this patch to
>> contact me for any question or clarification. Also, you might want to
>> take a look at the CSR [2].
>
> +1 on both counts.
>
>> As usual, Brian will make the patch available as webrev in the coming
>> hours.
>
> Please see
>
> http://cr.openjdk.java.net/~bpb/4511638/webrev.03/
>
> I wonder whether in the new AbstractStringBuilder.append() changes the
> constructs:
> 880 try {
> 881 FloatToDecimal.appendTo(f, this);
> 882 } catch (IOException ignored) {
> 883 assert false;
> 884 }
> might be better as:
> 880 try {
> 881 FloatToDecimal.appendTo(f, this);
> 882 } catch (IOException cause) {
> 883 throw new RuntimeException(cause);
> 884 }
> Comments appreciated.
>
> Brian


The reason is that FloatToDecimal.appendTo() takes an Appendable as
parameter. Appendable's append() methods throw a checked IOException.
This cannot happen with AbstractStringBuilder (the this passed as
argument), so the code catches the IOException just to please the
compiler. The assert is there just in case, but control shall never pass
there. Whatever conventions are in place to emphasize that control flow
cannot reach some point, I'll adopt them. Let me know.

The same holds for DoubleToDecimal.appendTo().

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Jason Mehrens]

Hi Goetz,

Looking at the test cases I didn't see any tests for the single argument 
java.util.Objects.requireNonNull.  Using this prototype is that method treated 
like a hidden frame?

Cheers,

Jason

From: core-libs-dev  on behalf of 
Lindenmaier, Goetz 
Sent: Friday, April 12, 2019 5:33 AM
To: Mandy Chung; Roger Riggs
Cc: Java Core Libs; hotspot-runtime-...@openjdk.java.net
Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException 
describing what is null.

Hi,

while waiting for progress on corresponding the JEP, I improved
the implementation of generating the NPE message.  It now uses
a single outputStream. This removes several allocations of temporary
data. I also removed TrackingStackSource. The analysis code originally
addressed several use cases, for NullPointerExceptions this is
not needed.  I cleaned up bytecodeUtils from some code not (really) needed.

I split get_null_pointer_slot() into two methods: get_NPE_null_slot()
and print_NPE_failed_action(). This simplifies the
implementation, and streamlines it more with the text in the JEP.

I print methods using the code added in
"8221470: Print methods in exception messages in java-like Syntax.",
so it now prints 'void m(int)' instead of 'm(I)V'.

I implemented a row of new test cases, and rearranged the test
to test the message part of print_NPE_failed_action() and
print_NPE_cause() separated. I made sure all bytecodes
handled in these methods are covered.
Further I arranged the tests in methods according to the
functional properties as discussed in the JEP.

http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/07

Best regards,
  Goetz.





> -Original Message-
> From: Lindenmaier, Goetz
> Sent: Donnerstag, 14. März 2019 21:56
> To: 'Mandy Chung' ; 'Roger Riggs'
> 
> Cc: 'Java Core Libs' ; 'hotspot-runtime-
> d...@openjdk.java.net' 
> Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException
> describing what is null.
>
> Hi,
>
> I had promised to work on a better wording of the messages.
>
> This I deliver with this webrev:
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/05-
> otherMessages/
>
> The test in the webrev is modified to just print messages along with the
> code that raised the messages.
>
> Please have a look at these files with test output contained in the webrev:
> Messages with debug information:
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/05-
> otherMessages/output_with_debug_info.txt
> Messages without debug information:
> http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/05-
> otherMessages/output_no_debug_info.txt
>
> Especially look at the first few messages, they point out the usefulness
> of this change.  They precisely say what was null in a chain of dereferences.
>
> Best regards,
>   Goetz.
>
>
> > -Original Message-
> > From: Lindenmaier, Goetz
> > Sent: Wednesday, February 13, 2019 10:09 AM
> > To: 'Mandy Chung' ; Roger Riggs
> > 
> > Cc: Java Core Libs ; hotspot-runtime-
> > d...@openjdk.java.net
> > Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException
> > describing what is null.
> >
> > Hi Mandy,
> >
> > Thanks for supporting my intend of adding the message as such!
> > I'll start implementing this in Java and come back with a webrev
> > in a while.
> >
> > In parallel, I would like to continue discussing the other
> > topics, e.g., the wording of the message. I will probably come up
> > with a separate webrev for that.
> >
> > Best regards,
> >   Goetz.
> >
> >
> >
> > > -Original Message-
> > > From: core-libs-dev  On Behalf
> > > Of Mandy Chung
> > > Sent: Tuesday, February 12, 2019 7:32 PM
> > > To: Roger Riggs 
> > > Cc: Java Core Libs ; hotspot-runtime-
> > > d...@openjdk.java.net
> > > Subject: Re: RFR(L): 8218628: Add detailed message to
> > NullPointerException
> > > describing what is null.
> > >
> > > On 2/8/19 11:46 AM, Roger Riggs wrote:
> > > > Hi,
> > > >
> > > > A few higher level issues should be considered, though the details
> > > > of the webrev captured my immediate attention.
> > > >
> > > > Is this the right feature and is this the right level of implementation
> > > > (C++/native)?
> > > > :
> > > > How much of this can be done in Java code with StackWalker and other
> > > > java APIs?
> > > > It would be a shame to add this much native code if there was a more
> > > robust
> > > > way to implement it using APIs with more leverage.
> > >
> > > Improving the NPE message for better diagnosability is helpful while
> > > I share the same concern Roger raised.
> > >
> > > Implementing this feature in Java and the library would be a better
> > > choice as this isn't absolutely required to be done in VM in native.
> > >
> > > NPE keeps a backtrace capturing the method id and bci of each stack
> > > frame.  One option to explore is to have StackWalker to accept a
> > > Throwable object that returns a stream of StackFrame which allows
> > > 

Re: RFR(L): 8218628: Add detailed message to NullPointerException describing what is null.

[Jason Mehrens]

Hi Goetz,

For NullPointerException, I don't think you want writeReplace() to call a 
public method that can be overridden.  You probably should create a private 
helper method that both getMessage and writeReplace() call into.

Jason


From: core-libs-dev  on behalf of 
Lindenmaier, Goetz 
Sent: Tuesday, March 12, 2019 10:04 AM
To: Peter Levart; Roger Riggs; Java Core Libs
Cc: hotspot-runtime-...@openjdk.java.net
Subject: RE: RFR(L): 8218628: Add detailed message to NullPointerException 
describing what is null.

Hi Peter,

>56 private static final String MESSAGE_PLACEHOLDER =
> "NPE_MESSAGE_PLACEHOLDER";
> Here, the initializer of that constant should create a private instance
> of String:
...
>  private static final String MESSAGE_PLACEHOLDER = new String();
Ok, I understand.

>81 public String getMessage() {
>82 String message = super.getMessage();
>83 if (message == MESSAGE_PLACEHOLDER) {
>84 message = getExtendedNPEMessage();
>85 setMessage(message);
>86 }
>87 return super.getMessage();
>88 }
>
> Why not simply returning 'message' local variable here?
As I removed the synchronization, this would reduce the chance to
get different objects in case of a race.  Highly unlikely though.

I changed the webrev with this solution to include these your remarks:
http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/03-PeterLevart/
(updated in-place).

Nevertheless, I think I will follow Roger's demand to not modify the 
defaultMessage
field.

Best regards,
  Goetz.


>
>
> Regards, Peter
>
> On 2/14/19 5:35 PM, Lindenmaier, Goetz wrote:
> > Hi Roger,
> >
> >> Maybe 10 years ago, when native was the only solution.
> >> Now there are tools to analyze bytecode in java.
> > I'm working on a Java implementation.
> >
> >>> Peter Levart proposed to initialize the message with a sentinel instead.
> >>> I'll implement this as a proposal.
> >> That's an extra overhead too, any assignment is.
> > Yes, any code requires some run time. But I think this really is negligible
> > in comparison to generating the backtrace, which happens on every
> > exception.  But I'm fine with the 'always recompute, no serialization'
> > solution. If it turns out to be limited, it still can be changed easily.
> >
> >>> I guess we can go without.
> >>> It would be possible to construct a case where two threads
> >>> do getMessage() on the same NPE Object but the string is not
> >>> the same.
> >> Really!, if the bci is the same, the bytecode is the same, what could be
> different
> >> that would change the data used to create the exception?
> > e.getMessage().equals(e.getMessage()) will hold, but
> > e.getMessage() != e.getMessage().
> >
> > I just implemented the two variants of computing the message, they
> > basically differ in NullPointerException.java:
> > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/03-
> PeterLevart/
> > http://cr.openjdk.java.net/~goetz/wr19/8218628-exMsg-NPE/04-
> RogerRiggs/
> >
> > I also cleaned up the parameters passed as proposed.
> >
> >>> We uses this code since 2006, it needed only few changes.  I would like to
> >>> contribute a follow up for hidden frames of lambdas.
> >> Valhalla and evolution of the byte code needs to be considered.
> >> Just because its worked for years does not mean it's the best approach
> >> today.  Dropping it in now means maintaining it in its current form
> >> or needing to re-write it later.
> > Well, yes, that is true. For any change.  For any project.  We have heard
> > this argument for many of our changes. I don't really think it's a good
> > argument. As I understand Valhalla is not targeted for jdk13, and
> > holding up changes for some future projects not really is the process
> > of OpenJDK, is it?
> >
>  The change to the jtreg/vmTestbase/jit/t/t104/t104.gold file raises a
>  question
>  about how useful the information is,  developers should not need to know
>  about "slot 1".
> >>> Developers should work with class files with debug information and then
> >>> they would get the name printed. If exceptions are thrown in production
> >>> code, any information is helpful.  Should I just write "a local"?
> >> Go back to who you think is going to benefit from it, it seems targeted
> >> at a fairly novice developer, so exposing low level details may be more
> >> confusing that just giving a line number and NPE.
> > Especially on our improved NPE messages we always got good feedback
> > from the developers within SAP. And there are quite some experienced
> > people.  Any message requires some background to be helpful.  And I
> > like to get any information I can have in first place. Attaching the
> > debugger often is a big overhead. E.g., I look at about 50 failing jtreg
> > tests every day, I don't want to get each into the debugger every time
> > in the setup where it was running to see what is 

Re: java.lang.CharSequence#compare - lexicographical ordering of custom Comparable CharSequence(s)

[Jason Mehrens]

Hi Peter,

The initial implementation was only optimized to call into String.compare if 
the arguments were string [1].  I proposed the current code a general form to 
catch java.nio.CharBuffer and any new JDK implementations of CharSequence + 
Comparable.

Naively, I lean towards "- CharSequence interface specification should be 
extended to require Comparable CharSeqeunces to implement lexicographical 
ordering".

Jason

1: 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2018-January/051124.html


From: core-libs-dev  on behalf of Peter 
Levart 
Sent: Monday, March 11, 2019 4:04 AM
To: core-libs-dev
Subject: java.lang.CharSequence#compare - lexicographical ordering of custom 
Comparable CharSequence(s)

Hello,

I stumbled on the specification of a static utility method
CharSequence#compare introduced in Java 11 which says:

  * Compares two {@code CharSequence} instances lexicographically.
Returns a
  * negative value, zero, or a positive value if the first sequence
is lexicographically
  * less than, equal to, or greater than the second, respectively.

The implementation of this method does the following:

 public static int compare(CharSequence cs1, CharSequence cs2) {
 if (Objects.requireNonNull(cs1) == Objects.requireNonNull(cs2)) {
 return 0;
 }

 if (cs1.getClass() == cs2.getClass() && cs1 instanceof
Comparable) {
 return ((Comparable) cs1).compareTo(cs2);
 }

 ... lexical comparison char-by-char ...


This means that if the method is called with two instances of the same
class that also implements Comparable, the comparison is delegated to
the .compareTo() method of that class. But CharSequence interface
neither extends Comparable nor does it specify  what such CharSequence
implementations should conform to when they also implement Comparable.
There could be a perfectly valid custom CharSequence implementation that
implemented Comparable which doesn't order instances lexicographically.
The guarantee this method gives is not respected in this case.

So, what shall be done?

- nothing
- CharSequence interface specification should be extended to require
Comparable CharSeqeunces to implement lexicographical ordering
- CharSequence#compare method specification should be extended to inform
the users about that delegation to Comparable CharSequence(s)
- CharSequence#compare method implementation should be changed to not
delegate to .compareTo() (at all or just for "unknown" Comparable
CharSequence(s))


Regards, Peter

Re: Proposal: ArrayList constructor perforrmance improvement

[Jason Mehrens]

>Sorry for not having remembered the history.

**Start the wavy motion effect because we are going back in time!

==
Date: Wed, 27 Sep 2006 16:49:47 -0700
From: Martin Buchholz 
Subject: 6347106 (coll) Make ArrayList(Collection) more threadsafe
Sender: 
To: Jason Mehrens 

Hi Jason,

Thanks for the SDN comment.
I updated 4759223 and 4918916.

I closed 4759223 as a dup of 6347106, but
4918916 is still an issue (although I should probably close as
Not a Defect, following Josh).
==

So most of the history is in the following:
https://bugs.openjdk.java.net/browse/JDK-4918916
https://bugs.openjdk.java.net/browse/JDK-4759223
https://bugs.openjdk.java.net/browse/JDK-6347106

Yes I still have this email and that Sun Ultra 20 from the Mustang Regressions 
challenge :)

Jason

Re: Proposal: ArrayList constructor perforrmance improvement

[Jason Mehrens]

The reason the patched code is faster is because it will avoid an array resize 
that is triggered in the benchmark by:
 "// once a new ArrayList is created add a new element
al.add(new Integer(900));"
http://cr.openjdk.java.net/~sgroeger/perf/arraylist/ArrayListBenchmark.java


If you look at the patch, it is over provisioning the backing array by using 
"elements.length" (elementData = new Object[((ArrayList)c).elementData.length];)
instead of "size".  The toArray call uses size and then the benchmark adds one 
element to trigger the resize.

Not sure if over provisioning the backing array is the right choice.  I tend to 
lean towards the current paradigm of exact sizing on copy.

Jason

Re: Proposal: ArrayList constructor perforrmance improvement

[Jason Mehrens]

Hi Steve,

>ArrayList has a constructor which takes an arbitrary Collection as a 
>parameter. This constructor will create an iterator over the collection 
>;and it will add each entry returned to the ArrayList. 

>We have found that quite a lot of the time the object passed as a 
>parameter is in fact an instance of ArrayList. 

>In the case of an ArrayList it is possible to significantly increase the 
>performance of the method since there is no need for an iterator - the 
>backing array can be directly copied.

Maybe I'm looking at a different version of the ArrayList source code but it 
seems that the ArrayList constructor calls c.toArray().  If the given 
Collection is an ArrayList then that will call the overridden ArrayList.toArray 
which directly copies the backing array once and doesn't create an iterator.  I 
would assume that most collections provide an optimized toArray().

Jason

Re: RFR JDK-8209553: ExceptionInInitializerError can have a default detail message if the cause is given

[Jason Mehrens]

Looks good.  I like the change of making setCause final.

Jason

From: mandy chung 
Sent: Thursday, September 13, 2018 3:50 PM
To: Jason Mehrens; joe darcy; Peter Levart
Cc: core-libs-dev
Subject: Re: RFR JDK-8209553: ExceptionInInitializerError can have a default 
detail message if the cause is given



On 9/13/18 12:47 PM, Jason Mehrens wrote:

Hi Mandy,

Like in previous patches, I advocated for using 'super.getCause()' in 
writeObject to avoid any subclass hooking into serialization.  I even lean 
towards the legacy getXXX methods call super.getCause too so they are 
compatible with previous behavior.


Good point.  It's probably rare to have subclasses of these types and overrides 
getCause method.  In any case I agree it should call super.getCause for 
correctness.  I also added a test case with overridden getCause method.

http://cr.openjdk.java.net/~mchung/jdk12/webrevs/8210721/webrev.01/


Does Throwable.setCause need a more obscure name encase there are subclasses of 
throwable in the wild with that signature?


This is package-private method.


EIIE has double semi-colon in the constructor and PAE has a random javadoc 
modification.



Good catch!  It's cleaned up.

Mandy

Re: RFR JDK-8209553: ExceptionInInitializerError can have a default detail message if the cause is given

[Jason Mehrens]

Hi Mandy,

Like in previous patches, I advocated for using 'super.getCause()' in 
writeObject to avoid any subclass hooking into serialization.  I even lean 
towards the legacy getXXX methods call super.getCause too so they are 
compatible with previous behavior.

Does Throwable.setCause need a more obscure name encase there are subclasses of 
throwable in the wild with that signature?

EIIE has double semi-colon in the constructor and PAE has a random javadoc 
modification.

Cheers,

Jason



From: core-libs-dev  on behalf of mandy 
chung 
Sent: Thursday, September 13, 2018 1:49 PM
To: joe darcy; Peter Levart
Cc: core-libs-dev
Subject: Re: RFR JDK-8209553: ExceptionInInitializerError can have a default 
detail message if the cause is given


On 8/16/18 4:48 PM, joe darcy wrote:
> On 8/15/2018 5:10 PM, mandy chung wrote:
>>
>>
>> On 8/15/18 3:20 PM, Peter Levart wrote:
>>> Hi Mandy,
>>>
>>> Just a question. Why does "private Throwable exception" field in
>>> ExceptionInInitializerError exist? Was it there before there was a
>>> "cause" in Throwable and later still remained there because of
>>> serialization format? Would it be possible to "simulate" its effect
>>> for serialization using "serialPersistentFields" and
>>> ObjectOutputStream.PutField?
>>
>> Thanks for asking.  I meant to mention this and it'd be nice to
>> follow up this in a separate issue.
>>
>> The private exception field exists since 1.1 and kept there for
>> serialization.  getException in existing releases returns the
>> exception field.  I can't think of any way to remove the exception
>> field in JDK n to deserialize it with older JDK x unless JDK x was
>> changed to write the exception field with the cause or getException
>> to return cause.
>
> Just a quick comment, it is possible, although a bit tedious, to
> remove a field and retain serial compatibility if
> readObject/writeObject methods are added to the new version of the
> class. There are a few examples of doing this kind of conversion in
> the JDK, such as for BigInteger.
>

Looking at the history, it turns out that these redundant fields were
removed at one point when the exception chaining support was initially
implemented but it got reverted to fix JDK-4385429.I have posted a
proposed patch to remove the private Throwable exception field and also
clean up a few other exception classes as a separate JBS issue.

http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-September/055415.html

Mandy

Re: RFR JDK-8209553: ExceptionInInitializerError can have a default detail message if the cause is given

[Jason Mehrens]

Hi Mandy,

This what we were doing in the past and has examples of removing fields with 
expected tests:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-February/051339.html
http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html

However, this case is a little different because it actually disallows 
subsequent initCause.  I would assume that will get tricky if you deserialize 
the old binary form in a newer JDK because we would have to jump through some 
hoops to update null to the cause exception.

Jason


From: core-libs-dev  on behalf of mandy 
chung 
Sent: Thursday, August 16, 2018 6:54 PM
To: joe darcy; Peter Levart
Cc: core-libs-dev
Subject: Re: RFR JDK-8209553: ExceptionInInitializerError can have a default 
detail message if the cause is given



On 8/16/18 4:48 PM, joe darcy wrote:
>>> Just a question. Why does "private Throwable exception" field in
>>> ExceptionInInitializerError exist? Was it there before there was a
>>> "cause" in Throwable and later still remained there because of
>>> serialization format? Would it be possible to "simulate" its effect
>>> for serialization using "serialPersistentFields" and
>>> ObjectOutputStream.PutField?
>>
>> Thanks for asking.  I meant to mention this and it'd be nice to
>> follow up this in a separate issue.
>>
>> The private exception field exists since 1.1 and kept there for
>> serialization.  getException in existing releases returns the
>> exception field.  I can't think of any way to remove the exception
>> field in JDK n to deserialize it with older JDK x unless JDK x was
>> changed to write the exception field with the cause or getException
>> to return cause.
>
> Just a quick comment, it is possible, although a bit tedious, to remove
> a field and retain serial compatibility if readObject/writeObject
> methods are added to the new version of the class. There are a few
> examples of doing this kind of conversion in the JDK, such as for
> BigInteger.


Thanks Joe.  In EIIE case, ideally we should remove the private
exception field and then write that to the serialize stream.
However, PutField::put requires the field to be present in
the current class; otherwise it throws IAE.

ObjectOutputStream.PutField fields = s.putFields();
fields.put("exception", getCause());

I haven't digged the history but I assume a field in BigInteger
was not renamed/removed.

Any other suggestion would be appreciated.

Mandy

Re: RFR JDK-8066709 Make some JDK system properties read only

[Jason Mehrens]

Roger,

Looks like StaticProperty.initProperty is never called.  I assume that was 
suppose to be called on lines 40-43?

Jason

From: core-libs-dev  on behalf of Roger 
Riggs 
Sent: Tuesday, June 5, 2018 9:18 AM
To: Stuart Marks
Cc: Core-Libs-Dev
Subject: Re: RFR JDK-8066709 Make some JDK system properties read only

Hi Stuart,

On 6/4/2018 9:52 PM, Stuart Marks wrote:
>
>
> On 6/4/18 6:32 AM, Roger Riggs wrote:
>> Please review a change to make the values of java.home, user.home,
>> user.dir, and user.name
>> effectively read-only for internal use.  The values are cached during
>> initialization and the
>> cached values are used.
>>
>> Webrev:
>> http://cr.openjdk.java.net/~rriggs/webrev-static-property-8066709/
>>
>> Issue:
>>https://bugs.openjdk.java.net/browse/JDK-8066709
>>
>> CSR:
>>https://bugs.openjdk.java.net/browse/JDK-8204235
>
> Hi Roger,
>
> Overall I think the intent of the change is a good one, as is the
> reimplementation of internal uses of system properties to use an
> internal API instead.
>
> I think it would be good to have an explicit initialization of the
> SystemProperty class (or whatever it ends up being called) instead of
> implicitly initializing via a static initializer. If the class is
> initialized too early, for some reason, things would go wrong.
>
That would be a no-op;  I want the values of the properties to be final
statics which means they have to
be initialized by the static initializer.
> Should there be an error, a warning, or assertion checking to make
> sure that none of the cached values are null? They should all be
> non-null, right?
Yes, null will be a fatal InternalError.

Thanks, Roger

>
> s'marks

Re: RFR (JDK11) 8137326: Methods for comparing CharSequence, StringBuilder, and StringBuffer

[Jason Mehrens]

Looks good.

Jason

From: Joe Wang <huizhe.w...@oracle.com>
Sent: Monday, February 12, 2018 12:25 PM
To: Jason Mehrens
Cc: core-libs-dev
Subject: Re: RFR (JDK11) 8137326: Methods for comparing CharSequence, 
StringBuilder, and StringBuffer

Done. Thanks Jason!

Joe


On 2/9/18, 1:46 PM, Jason Mehrens wrote:
> Joe,
> In CharSequence, does it make sense to cache the result of the length 
> calculation?
> As in change:
> 
> for (int i = 0; i<  Math.min(cs1.length(), cs2.length()); i++) {
> 
> for (int i = 0, len = Math.min(cs1.length(), cs2.length()); i<  len; i++) {
> 
>
> Jason
> 
> From: core-libs-dev<core-libs-dev-boun...@openjdk.java.net>  on behalf of Joe 
> Wang<huizhe.w...@oracle.com>
> Sent: Thursday, February 8, 2018 6:47 PM
> To: core-libs-dev
> Subject: Re: RFR (JDK11) 8137326: Methods for comparing CharSequence, 
> StringBuilder, and StringBuffer
>
> Hi all,
>
> The CSR for the enhancement is now approved. Thanks Joe!
>
> The webrev has been updated accordingly. Please let me know if you have
> any further comment on the implementation.
> JBS: https://bugs.openjdk.java.net/browse/JDK-8137326
> webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/
>
> Thanks,
> Joe
>
> On 2/2/2018 12:46 PM, Joe Wang wrote:
>> Thanks Jason. Will update that accordingly.
>>
>> Best,
>> Joe
>>
>> On 2/2/2018 11:22 AM, Jason Mehrens wrote:
>>> Joe,
>>>
>>> The identity check in CS.compare makes sense.  However, it won't be
>>> null hostile if we call CS.compare(null, null) and that doesn't seem
>>> right.
>>> Usually when writing comparator classes I end up with:
>>> ===
>>> if (Objects.requireNonNull(o1) == Objects.requireNonNull(o2)) {
>>>   return 0;
>>> }
>>> ===
>>>
>>> Jason
>>> 
>>> From: core-libs-dev<core-libs-dev-boun...@openjdk.java.net>  on
>>> behalf of Joe Wang<huizhe.w...@oracle.com>
>>> Sent: Friday, February 2, 2018 1:01 PM
>>> To: core-libs-dev
>>> Subject: Re: RFR (JDK11) 8137326: Methods for comparing CharSequence,
>>> StringBuilder, and StringBuffer
>>>
>>> Hi,
>>>
>>> Thanks all for comments and suggestions. I've updated the webrev. Please
>>> review.
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8137326
>>> webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/
>>>
>>> Thanks,
>>> Joe
>>>
>>> On 1/31/2018 9:31 PM, Joe Wang wrote:
>>>> Hi Tagir,
>>>>
>>>> Thanks for the comment. I will consider adding that to the javadoc
>>>> emphasizing that the comparison is performed from 0 to length() - 1 of
>>>> the two sequences.
>>>>
>>>> Best,
>>>> Joe
>>>>
>>>> On 1/29/18, 8:07 PM, Tagir Valeev wrote:
>>>>> Hello!
>>>>>
>>>>> An AbstractStringBuilder#compareTo implementation is wrong. You cannot
>>>>> simply compare the whole byte array. Here's the test-case:
>>>>>
>>>>> public class Test {
>>>>>  public static void main(String[] args) {
>>>>>StringBuilder sb1 = new StringBuilder("test1");
>>>>>StringBuilder sb2 = new StringBuilder("test2");
>>>>>sb1.setLength(4);
>>>>>sb2.setLength(4);
>>>>>System.out.println(sb1.compareTo(sb2));
>>>>> System.out.println(sb1.toString().compareTo(sb2.toString()));
>>>>>  }
>>>>> }
>>>>>
>>>>> We truncated the stringbuilders making their content equal, so
>>>>> sb1.toString().compareTo(sb2.toString()) is 0, but compareTo compares
>>>>> the original content (before the truncation) as truncation, of course,
>>>>> does not zero the truncated bytes, neither does it reallocate the
>>>>> array (unless explicitly asked via trimToSize).
>>>>>
>>>>> With best regards,
>>>>> Tagir Valeev.
>>>>>
>>>>>
>>>>> On Fri, Jan 26, 2018 at 10:00 AM, Joe Wang<huizhe.w...@oracle.com>
>>>>> wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Adding methods for comparing CharSequence, StringBuilder, and
>>>>>> StringBuffer.
>>>>>>
>>>>>> The Comparable implementations for StringBuilder/Buffer are similar
>>>>>> to that
>>>>>> of String, allowing comparison operations between two
>>>>>> StringBuilders/Buffers, e.g.
>>>>>> aStringBuilder.compareTo(anotherStringBuilder).
>>>>>> For CharSequence however, refer to the comments in JIRA, a static
>>>>>> method
>>>>>> 'compare' is added instead of implementing the Comparable interface.
>>>>>> This
>>>>>> 'compare' method may take CharSequence implementations such as
>>>>>> String,
>>>>>> StringBuilder and StringBuffer, making it possible to perform
>>>>>> comparison
>>>>>> among them. The previous example for example is equivalent to
>>>>>> CharSequence.compare(aStringBuilder, anotherStringBuilder).
>>>>>>
>>>>>> Tests for java.base have been independent from each other. The new
>>>>>> tests are
>>>>>> therefore created to have no dependency on each other or sharing any
>>>>>> code.
>>>>>>
>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8137326
>>>>>> webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/
>>>>>>
>>>>>> Thanks,
>>>>>> Joe

Re: RFR (JDK11) 8137326: Methods for comparing CharSequence, StringBuilder, and StringBuffer

[Jason Mehrens]

Joe,
In CharSequence, does it make sense to cache the result of the length 
calculation?
As in change:

for (int i = 0; i < Math.min(cs1.length(), cs2.length()); i++) {

for (int i = 0, len = Math.min(cs1.length(), cs2.length()); i < len; i++) {


Jason

From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on behalf of Joe 
Wang <huizhe.w...@oracle.com>
Sent: Thursday, February 8, 2018 6:47 PM
To: core-libs-dev
Subject: Re: RFR (JDK11) 8137326: Methods for comparing CharSequence, 
StringBuilder, and StringBuffer

Hi all,

The CSR for the enhancement is now approved. Thanks Joe!

The webrev has been updated accordingly. Please let me know if you have
any further comment on the implementation.
JBS: https://bugs.openjdk.java.net/browse/JDK-8137326
webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/

Thanks,
Joe

On 2/2/2018 12:46 PM, Joe Wang wrote:
> Thanks Jason. Will update that accordingly.
>
> Best,
> Joe
>
> On 2/2/2018 11:22 AM, Jason Mehrens wrote:
>> Joe,
>>
>> The identity check in CS.compare makes sense.  However, it won't be
>> null hostile if we call CS.compare(null, null) and that doesn't seem
>> right.
>> Usually when writing comparator classes I end up with:
>> ===
>> if (Objects.requireNonNull(o1) == Objects.requireNonNull(o2)) {
>>  return 0;
>> }
>> ===
>>
>> Jason
>> 
>> From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on
>> behalf of Joe Wang <huizhe.w...@oracle.com>
>> Sent: Friday, February 2, 2018 1:01 PM
>> To: core-libs-dev
>> Subject: Re: RFR (JDK11) 8137326: Methods for comparing CharSequence,
>> StringBuilder, and StringBuffer
>>
>> Hi,
>>
>> Thanks all for comments and suggestions. I've updated the webrev. Please
>> review.
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8137326
>> webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/
>>
>> Thanks,
>> Joe
>>
>> On 1/31/2018 9:31 PM, Joe Wang wrote:
>>> Hi Tagir,
>>>
>>> Thanks for the comment. I will consider adding that to the javadoc
>>> emphasizing that the comparison is performed from 0 to length() - 1 of
>>> the two sequences.
>>>
>>> Best,
>>> Joe
>>>
>>> On 1/29/18, 8:07 PM, Tagir Valeev wrote:
>>>> Hello!
>>>>
>>>> An AbstractStringBuilder#compareTo implementation is wrong. You cannot
>>>> simply compare the whole byte array. Here's the test-case:
>>>>
>>>> public class Test {
>>>> public static void main(String[] args) {
>>>>   StringBuilder sb1 = new StringBuilder("test1");
>>>>   StringBuilder sb2 = new StringBuilder("test2");
>>>>   sb1.setLength(4);
>>>>   sb2.setLength(4);
>>>>   System.out.println(sb1.compareTo(sb2));
>>>> System.out.println(sb1.toString().compareTo(sb2.toString()));
>>>> }
>>>> }
>>>>
>>>> We truncated the stringbuilders making their content equal, so
>>>> sb1.toString().compareTo(sb2.toString()) is 0, but compareTo compares
>>>> the original content (before the truncation) as truncation, of course,
>>>> does not zero the truncated bytes, neither does it reallocate the
>>>> array (unless explicitly asked via trimToSize).
>>>>
>>>> With best regards,
>>>> Tagir Valeev.
>>>>
>>>>
>>>> On Fri, Jan 26, 2018 at 10:00 AM, Joe Wang<huizhe.w...@oracle.com>
>>>> wrote:
>>>>> Hi,
>>>>>
>>>>> Adding methods for comparing CharSequence, StringBuilder, and
>>>>> StringBuffer.
>>>>>
>>>>> The Comparable implementations for StringBuilder/Buffer are similar
>>>>> to that
>>>>> of String, allowing comparison operations between two
>>>>> StringBuilders/Buffers, e.g.
>>>>> aStringBuilder.compareTo(anotherStringBuilder).
>>>>> For CharSequence however, refer to the comments in JIRA, a static
>>>>> method
>>>>> 'compare' is added instead of implementing the Comparable interface.
>>>>> This
>>>>> 'compare' method may take CharSequence implementations such as
>>>>> String,
>>>>> StringBuilder and StringBuffer, making it possible to perform
>>>>> comparison
>>>>> among them. The previous example for example is equivalent to
>>>>> CharSequence.compare(aStringBuilder, anotherStringBuilder).
>>>>>
>>>>> Tests for java.base have been independent from each other. The new
>>>>> tests are
>>>>> therefore created to have no dependency on each other or sharing any
>>>>> code.
>>>>>
>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8137326
>>>>> webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/
>>>>>
>>>>> Thanks,
>>>>> Joe
>

Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

[Jason Mehrens]

Aleksei,

Looks good to me.

Jason

From: Aleks Efimov <aleksej.efi...@oracle.com>
Sent: Wednesday, February 7, 2018 7:24 PM
To: Roger Riggs; Jason Mehrens
Cc: core-libs-dev@openjdk.java.net
Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not 
correctly set Exception

Hi Jason, Roger,

Thanks for your comments.
The new webrev with suggested/recommended edits can be viewed at this
location:
http://cr.openjdk.java.net/~aefimov/6857903/dev/03/

Thanks,
Aleksei

PS: Also configured my IDE to use explicit imports =)

On 02/06/2018 03:31 PM, Roger Riggs wrote:
> Hi Aleksei,
>
> SAXException.java x 2, SAXExceptionInitCause:
>   Please use explicit imports (not java.io.*).
>
> SAXEXception.java:145: I would have expected:
> if (cause instanceof Exception) {...}
>
> SAXExceptionInitCause.java:
>   Great to see all the test cases.
>
>   Generally, we recommend using try-with-resources but in this case it
> does not add much because
>   exceptions can't occur when using BAOS.
>
> Thanks, Roger
>
> On 2/5/2018 9:46 PM, Aleks Efimov wrote:
>> Thank you for your comments, Jason!
>> New webrev can be found at this location:
>> http://cr.openjdk.java.net/~aefimov/6857903/dev/02/
>>
>> The list of changes:
>> - this.initCause() -> super.initCause()
>> - 'readObject' has been modified to convert IllegalStateException to
>> InvalidClassException. The test case that triggers
>> IllegalStateException in SAXException::readObject has been added to
>> 'testReadObjectIllegalStateException' test method.
>>
>> Best Regards,
>> Aleksei
>>
>> On 02/05/2018 05:09 PM, Jason Mehrens wrote:
>>> Aleksei,
>>>
>>> Looks good.  We should avoid this.initCause in readObject and prefer
>>> super.initCause so subclasses can't alter the deserialization behavior.
>>> While I can't think of a case off the top of my head, I would prefer
>>> that we trap and convert IllegalStateException into a
>>> InvalidClassException in readObject.
>>> That keeps the readObject contract intact in case something
>>> malicious is going on.  That is just my preference though.
>>>
>>> Jason
>>> 
>>> From: Aleks Efimov <aleksej.efi...@oracle.com>
>>> Sent: Monday, February 5, 2018 10:51 AM
>>> To: Jason Mehrens; 'Roger Riggs'
>>> Cc: core-libs-dev
>>> Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does
>>> not correctly set Exception
>>>
>>> Hi Roger, Jason,
>>>
>>> I've tried to avoid doing the same stuff as I did for XPathException to
>>> minimize the changes to the SAXException class. But I was naive to
>>> think so:
>>> Tried to keep the exception field in place to avoid modifications
>>> required to keep serial form intact (similar as it was done in
>>> JDK-8009581 bug mentioned by Jason), but I missed the 'initCause'
>>> method
>>> (spotted by Roger, thank you!) that can make cause/exception values
>>> inconsistent. To avoid the API modification and for the sake of clarity
>>> I proceeded with removal of the 'exception' field. The new version of
>>> the fix:
>>> - Removes 'exception' field
>>> - Maintains the serial form of the SAXException class
>>> - Handles the difference in SAXException:exception and Throwable:cause
>>> types 'SAXException::getException', i.e. SAXException:exception is
>>> Exception typed and Throwable:cause is Throwable typed.
>>> - Avoids any changes to API and serial form of the class, but
>>> maintaining it similar to JDK-8009581)
>>> - There is a copy of SAXException class in java.base module that is
>>> required for j.u.Properties::loadFromXML, it has been update with the
>>> same changes.
>>>
>>> New regression test has been added to test:
>>> - Serial compatibility with legacy JDKs (similar to JDK-8009581)
>>> - usages of SAXException::initCause/getException
>>>
>>> Webrev with the fix and new regression:
>>> http://cr.openjdk.java.net/~aefimov/6857903/dev/01/
>>>
>>> Testing shows no JCK/regression tests failures with the proposed fix.
>>>
>>> Best Regards,
>>> Aleksei
>>>
>>>
>>> On 12/21/2017 07:00 PM, Jason Mehrens wrote:
>>>> Aleksei,
>>>>
>>>> You have to override all of the constructors to always disable
>>>> initCause.  Actually a similar issue was covered in:
>>>>
>>

Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

[Jason Mehrens]

Aleksei,

I missed this before but, it looks like we are calling the public getException 
method in writeObject.  We should create a private method to service both the 
public getException method and writeObject to guard against subclass tampering.

Everything else looks good.

Jason

From: Aleks Efimov <aleksej.efi...@oracle.com>
Sent: Monday, February 5, 2018 8:46 PM
To: Jason Mehrens
Cc: core-libs-dev
Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not 
correctly set Exception

Thank you for your comments, Jason!
New webrev can be found at this location:
http://cr.openjdk.java.net/~aefimov/6857903/dev/02/

The list of changes:
- this.initCause() -> super.initCause()
- 'readObject' has been modified to convert IllegalStateException to
InvalidClassException. The test case that triggers IllegalStateException
in SAXException::readObject has been added to
'testReadObjectIllegalStateException' test method.

Best Regards,
Aleksei

On 02/05/2018 05:09 PM, Jason Mehrens wrote:
> Aleksei,
>
> Looks good.  We should avoid this.initCause in readObject and prefer 
> super.initCause so subclasses can't alter the deserialization behavior.
> While I can't think of a case off the top of my head, I would prefer that we 
> trap and convert IllegalStateException into a InvalidClassException in 
> readObject.
> That keeps the readObject contract intact in case something malicious is 
> going on.  That is just my preference though.
>
> Jason
> 
> From: Aleks Efimov <aleksej.efi...@oracle.com>
> Sent: Monday, February 5, 2018 10:51 AM
> To: Jason Mehrens; 'Roger Riggs'
> Cc: core-libs-dev
> Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not 
> correctly set Exception
>
> Hi Roger, Jason,
>
> I've tried to avoid doing the same stuff as I did for XPathException to
> minimize the changes to the SAXException class. But I was naive to think so:
> Tried to keep the exception field in place to avoid modifications
> required to keep serial form intact (similar as it was done in
> JDK-8009581 bug mentioned by Jason), but I missed the 'initCause' method
> (spotted by Roger, thank you!) that can make cause/exception values
> inconsistent. To avoid the API modification and for the sake of clarity
> I proceeded with removal of the 'exception' field. The new version of
> the fix:
> - Removes 'exception' field
> - Maintains the serial form of the SAXException class
> - Handles the difference in SAXException:exception and Throwable:cause
> types 'SAXException::getException', i.e. SAXException:exception is
> Exception typed and Throwable:cause is Throwable typed.
> - Avoids any changes to API and serial form of the class, but
> maintaining it similar to JDK-8009581)
> - There is a copy of SAXException class in java.base module that is
> required for j.u.Properties::loadFromXML, it has been update with the
> same changes.
>
> New regression test has been added to test:
> - Serial compatibility with legacy JDKs (similar to JDK-8009581)
> - usages of SAXException::initCause/getException
>
> Webrev with the fix and new regression:
> http://cr.openjdk.java.net/~aefimov/6857903/dev/01/
>
> Testing shows no JCK/regression tests failures with the proposed fix.
>
> Best Regards,
> Aleksei
>
>
> On 12/21/2017 07:00 PM, Jason Mehrens wrote:
>> Aleksei,
>>
>> You have to override all of the constructors to always disable initCause.  
>> Actually a similar issue was covered in:
>>
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-May/016908.html
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html
>>
>> Pretty much everything was hashed out in those threads.
>>
>> Here is the final webrev for that:
>>
>> http://cr.openjdk.java.net/~dmeetry/8009581/webrev.5/jaxp/src/javax/xml/xpath/XPathException.java.udiff.html
>>
>> That should be a good cookbook for changes and tests required for this 
>> issue.  Note that it gets rid of the Exception field and maintains serial 
>> compatibility.
>> Looking back on that change, I would have changed it so the 
>> InvalidClassException added the double cause as suppressed exceptions.
>>
>> Jason
>>
>> 
>> From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on behalf of 
>> Aleks Efimov <aleksej.efi...@oracle.com>
>> Sent: Thursday, December 21, 2017 11:27 AM
>> To: core-libs-dev
>> Subject: RFR [11] (JAXP): 6857903: SAXException.initCause() does not 
>> correctly set Exception
>>
>> Hello,
>>
>> Please, help to review the fix f

Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

[Jason Mehrens]

Aleksei,

Looks good.  We should avoid this.initCause in readObject and prefer 
super.initCause so subclasses can't alter the deserialization behavior.
While I can't think of a case off the top of my head, I would prefer that we 
trap and convert IllegalStateException into a InvalidClassException in 
readObject.
That keeps the readObject contract intact in case something malicious is going 
on.  That is just my preference though.

Jason

From: Aleks Efimov <aleksej.efi...@oracle.com>
Sent: Monday, February 5, 2018 10:51 AM
To: Jason Mehrens; 'Roger Riggs'
Cc: core-libs-dev
Subject: Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not 
correctly set Exception

Hi Roger, Jason,

I've tried to avoid doing the same stuff as I did for XPathException to
minimize the changes to the SAXException class. But I was naive to think so:
Tried to keep the exception field in place to avoid modifications
required to keep serial form intact (similar as it was done in
JDK-8009581 bug mentioned by Jason), but I missed the 'initCause' method
(spotted by Roger, thank you!) that can make cause/exception values
inconsistent. To avoid the API modification and for the sake of clarity
I proceeded with removal of the 'exception' field. The new version of
the fix:
- Removes 'exception' field
- Maintains the serial form of the SAXException class
- Handles the difference in SAXException:exception and Throwable:cause
types 'SAXException::getException', i.e. SAXException:exception is
Exception typed and Throwable:cause is Throwable typed.
- Avoids any changes to API and serial form of the class, but
maintaining it similar to JDK-8009581)
- There is a copy of SAXException class in java.base module that is
required for j.u.Properties::loadFromXML, it has been update with the
same changes.

New regression test has been added to test:
- Serial compatibility with legacy JDKs (similar to JDK-8009581)
- usages of SAXException::initCause/getException

Webrev with the fix and new regression:
http://cr.openjdk.java.net/~aefimov/6857903/dev/01/

Testing shows no JCK/regression tests failures with the proposed fix.

Best Regards,
Aleksei


On 12/21/2017 07:00 PM, Jason Mehrens wrote:
> Aleksei,
>
> You have to override all of the constructors to always disable initCause.  
> Actually a similar issue was covered in:
>
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-May/016908.html
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html
>
> Pretty much everything was hashed out in those threads.
>
> Here is the final webrev for that:
>
> http://cr.openjdk.java.net/~dmeetry/8009581/webrev.5/jaxp/src/javax/xml/xpath/XPathException.java.udiff.html
>
> That should be a good cookbook for changes and tests required for this issue. 
>  Note that it gets rid of the Exception field and maintains serial 
> compatibility.
> Looking back on that change, I would have changed it so the 
> InvalidClassException added the double cause as suppressed exceptions.
>
> Jason
>
> 
> From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on behalf of 
> Aleks Efimov <aleksej.efi...@oracle.com>
> Sent: Thursday, December 21, 2017 11:27 AM
> To: core-libs-dev
> Subject: RFR [11] (JAXP): 6857903: SAXException.initCause() does not 
> correctly set Exception
>
> Hello,
>
> Please, help to review the fix for jaxp bug that fixes SAXException to
> correctly set exception cause with 'setCause' method:
> http://cr.openjdk.java.net/~aefimov/6857903/dev/00/
> I've tried to keep the fix miminal with respect to serial form of this
> API class, i.e. kept private 'exception' field.
>
> The new test case was added to IssueTracker56Test unit test. Testing
> showed no related JCK/JTREG failures.
>
> With Best Regards,
> Aleksei
>
>

Re: RFR (JDK11) 8137326: Methods for comparing CharSequence, StringBuilder, and StringBuffer

[Jason Mehrens]

Joe,

The identity check in CS.compare makes sense.  However, it won't be null 
hostile if we call CS.compare(null, null) and that doesn't seem right.
Usually when writing comparator classes I end up with:
===
if (Objects.requireNonNull(o1) == Objects.requireNonNull(o2)) {
return 0;
}
===

Jason

From: core-libs-dev  on behalf of Joe 
Wang 
Sent: Friday, February 2, 2018 1:01 PM
To: core-libs-dev
Subject: Re: RFR (JDK11) 8137326: Methods for comparing CharSequence, 
StringBuilder, and StringBuffer

Hi,

Thanks all for comments and suggestions. I've updated the webrev. Please
review.

JBS: https://bugs.openjdk.java.net/browse/JDK-8137326
webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/

Thanks,
Joe

On 1/31/2018 9:31 PM, Joe Wang wrote:
> Hi Tagir,
>
> Thanks for the comment. I will consider adding that to the javadoc
> emphasizing that the comparison is performed from 0 to length() - 1 of
> the two sequences.
>
> Best,
> Joe
>
> On 1/29/18, 8:07 PM, Tagir Valeev wrote:
>> Hello!
>>
>> An AbstractStringBuilder#compareTo implementation is wrong. You cannot
>> simply compare the whole byte array. Here's the test-case:
>>
>> public class Test {
>>public static void main(String[] args) {
>>  StringBuilder sb1 = new StringBuilder("test1");
>>  StringBuilder sb2 = new StringBuilder("test2");
>>  sb1.setLength(4);
>>  sb2.setLength(4);
>>  System.out.println(sb1.compareTo(sb2));
>> System.out.println(sb1.toString().compareTo(sb2.toString()));
>>}
>> }
>>
>> We truncated the stringbuilders making their content equal, so
>> sb1.toString().compareTo(sb2.toString()) is 0, but compareTo compares
>> the original content (before the truncation) as truncation, of course,
>> does not zero the truncated bytes, neither does it reallocate the
>> array (unless explicitly asked via trimToSize).
>>
>> With best regards,
>> Tagir Valeev.
>>
>>
>> On Fri, Jan 26, 2018 at 10:00 AM, Joe Wang 
>> wrote:
>>> Hi,
>>>
>>> Adding methods for comparing CharSequence, StringBuilder, and
>>> StringBuffer.
>>>
>>> The Comparable implementations for StringBuilder/Buffer are similar
>>> to that
>>> of String, allowing comparison operations between two
>>> StringBuilders/Buffers, e.g.
>>> aStringBuilder.compareTo(anotherStringBuilder).
>>> For CharSequence however, refer to the comments in JIRA, a static
>>> method
>>> 'compare' is added instead of implementing the Comparable interface.
>>> This
>>> 'compare' method may take CharSequence implementations such as String,
>>> StringBuilder and StringBuffer, making it possible to perform
>>> comparison
>>> among them. The previous example for example is equivalent to
>>> CharSequence.compare(aStringBuilder, anotherStringBuilder).
>>>
>>> Tests for java.base have been independent from each other. The new
>>> tests are
>>> therefore created to have no dependency on each other or sharing any
>>> code.
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8137326
>>> webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/
>>>
>>> Thanks,
>>> Joe

Re: RFR (JDK11) 8137326: Methods for comparing CharSequence, StringBuilder, and StringBuffer

[Jason Mehrens]

Joe,

1. Seems odd that StringBuilder and StringBuffer are comparable but don't have 
an equals/hashcode implementation.
Seems like if you are to stick with that you'll have to add the standard 
"natural ordering is inconsistent with equals" verbiage.
2. For StringBuffer compare, won't you have to either lock the 'another' object 
too so it is not mutated or perform some
sort of copy before compare? Neither sound like a good option.
3. Does CharSequence.compare need to specify that IndexOutOfBoundsException may 
be thrown under concurrent modification?
4. For CharSequence.compare, instead of creating a special case for string 
would it make sense to do:


if (cs1.getClass() == cs2.getClass() && cs1 instanceof Comparable) {
return ((Comparable) cs1).compareTo(cs2);
 }


Given #1 and #2 maybe StringBuilder and StringBuffer shouldn't implement 
comparable and just rely on users either calling
 'sb1.toString().compare(sb2.toString())' or 'CharSequence.compare(sb1, sb2)'.

Jason

From: core-libs-dev  on behalf of Joe 
Wang 
Sent: Thursday, January 25, 2018 9:00 PM
To: core-libs-dev
Subject: RFR (JDK11) 8137326: Methods for comparing CharSequence, 
StringBuilder, and StringBuffer

Hi,

Adding methods for comparing CharSequence, StringBuilder, and StringBuffer.

The Comparable implementations for StringBuilder/Buffer are similar to
that of String, allowing comparison operations between two
StringBuilders/Buffers, e.g.
aStringBuilder.compareTo(anotherStringBuilder). For CharSequence
however, refer to the comments in JIRA, a static method 'compare' is
added instead of implementing the Comparable interface. This 'compare'
method may take CharSequence implementations such as String,
StringBuilder and StringBuffer, making it possible to perform comparison
among them. The previous example for example is equivalent to
CharSequence.compare(aStringBuilder, anotherStringBuilder).

Tests for java.base have been independent from each other. The new tests
are therefore created to have no dependency on each other or sharing any
code.

JBS: https://bugs.openjdk.java.net/browse/JDK-8137326
webrev: http://cr.openjdk.java.net/~joehw/jdk11/8137326/webrev/

Thanks,
Joe

Re: [JDK 11] RFR: 8195096: Exception printed on console with custom LogManager on starting Apache Tomcat

[Jason Mehrens]

Looks great!  Personally, these LogManager startup issues make my head hurt. :)

Jason

From: Daniel Fuchs <daniel.fu...@oracle.com>
Sent: Tuesday, January 23, 2018 1:13 PM
To: Jason Mehrens; core-libs-dev@openjdk.java.net
Cc: Mandy Chung
Subject: Re: [JDK 11] RFR: 8195096: Exception printed on console with custom 
LogManager on starting Apache Tomcat

Hi Jason,

I take it back :-)

Mandy helped me finding better names to refactor the code, so
here is a (hopefully) better version:

http://cr.openjdk.java.net/~dfuchs/webrev_8195096/webrev.02/

best regards,

-- daniel

On 23/01/2018 10:27, Daniel Fuchs wrote:
> Hi Jason,
>
> On 22/01/2018 21:30, Jason Mehrens wrote:
>> Daniel,
>>
>> Fantastic!  The patch looks good as is.  The only > thing that sticks
>> out is that both the 'if' and
>> the 'else if' have the same code inside them.
>> That could be refactored a bit.
>
> I had the same feeling - but then when looking at
> possible refactoring I came to the conclusion that
> I liked the explicit if / else if / better.
>
> The duplicated code is just one line + one return
> so it's not that bad.
>
> best regards,
>
> -- daniel
>
>>
>> Jason
>

Re: [JDK 11] RFR: 8195096: Exception printed on console with custom LogManager on starting Apache Tomcat

[Jason Mehrens]

Daniel,

Fantastic!  The patch looks good as is.  The only thing that sticks out is that 
both the 'if' and the 'else if' have the same code inside them.
That could be refactored a bit.

Jason

From: core-libs-dev  on behalf of 
Daniel Fuchs 
Sent: Monday, January 22, 2018 10:47 AM
To: Mark Thomas; core-libs-dev@openjdk.java.net
Subject: Re: [JDK 11] RFR: 8195096: Exception printed on console with custom 
LogManager on starting Apache Tomcat

Hi guys,

Here is my second take at solving the issue.
There's no change in the public API (no CSR, Yay!),
and it shouldn't require any changes on the Tomcat
side. However I haven't tested my changes with Tomcat
yet.

Mark - would you be able to help me with that?

http://cr.openjdk.java.net/~dfuchs/webrev_8195096/webrev.01/

best regards,

-- daniel

On 20/01/2018 13:17, Daniel Fuchs wrote:
> Hi Mark,
>
> Thanks for these links and for trying out a solution.
>
>  > On 19/01/18 20:14, Mark Thomas wrote:
>  > Thanks again for the hints. I have a work-around that seems to work:
>  > https://svn.apache.org/viewvc?view=revision=1821708
>
> I see... so what I think is happening is that in JDK 8, the
> handlers from ".handlers" where added to the root logger from
> within addLogger(rootLogger) and because ClassLoaderLogManager
> overrides LogManager::addLogger, then that part was skipped.
>
> In JDK 9 - that behavior changed - the handlers from ".handlers"
> were no longer added because the root logger was already present
> in the context when addLogger got called (that was a regression),
> but since  ClassLoaderLogManager overrides LogManager::addLogger
> anyway - it made no difference for ClassLoaderLogManager.
>
> In JDK 10, to fix the issue, I forced a call that would
> add the handlers from ".handlers" to the root logger, but
> because this now happens outside of addLogger(rootLogger) (just after)
> then ClassLoaderLogManager feels the difference.
>
> hmmm... let me think on this a bit more.
>
> best regards,
>
> -- daniel

Re: [JDK 11] RFR: 8195096: Exception printed on console with custom LogManager on starting Apache Tomcat

[Jason Mehrens]

Not my call but adding addInitialRootLoggerHandlers() seems too much like a 
private detail that would be forever published as part of the public API.

I would rather have have a protected reportError added to LogManager and 
redirect all internal errors from directly calling System.err to that method.
Then sub-classes could override that method and ignore internal errors 
generated by the super class.

Jason


From: core-libs-dev  on behalf of Mark 
Thomas 
Sent: Friday, January 19, 2018 2:56 PM
To: core-libs-dev@openjdk.java.net
Subject: Re: [JDK 11] RFR: 8195096: Exception printed on console with custom 
LogManager on starting Apache Tomcat

On 19/01/18 20:14, Mark Thomas wrote:



> Thanks to both of you for the hints that have got me thinking in new
> directions for a workaround.

Thanks again for the hints. I have a work-around that seems to work:
https://svn.apache.org/viewvc?view=revision=1821708

It feels a bit hacky. I'd still prefer to be able to override
addInitialRootLoggerHandlers() as that strikes me as a cleaner solution.

Cheers,

Mark

Re: [JDK 11] RFR: 8195096: Exception printed on console with custom LogManager on starting Apache Tomcat

[Jason Mehrens]

Daniel,

Double check me on this but, I think I've found the source of why the error 
occurs on JDK9 and not JDK8.
In https://bugs.openjdk.java.net/browse/JDK-8191033 the new code should have 
been added to initializeGlobalHandlers.  That would make the read of ".handers" 
lazy and keep the read of "handlers" eager.

Jason

From: Daniel Fuchs <daniel.fu...@oracle.com>
Sent: Friday, January 19, 2018 4:13 AM
To: Jason Mehrens; core-libs-dev
Subject: Re: [JDK 11] RFR: 8195096: Exception printed on console with custom 
LogManager on starting Apache Tomcat

Hi Jason,

On 18/01/2018 21:19, Jason Mehrens wrote:
> Daniel,
>
> As long as the org.apache.juli.ClassLoaderLogManager overrides getProperty it 
> shouldn't really matter what the value format is in the file as long as it is 
> translated on return.
> Is there a code path in j.u.l.LogManager that doesn't call getProperty?  If 
> so I would think that is the core issue.

That's a very good question. Why didn't I think of it?

AFAICS there's only one place where we do that, in the
'new' updateConfiguration method, and that's only for
levels when a level value is changed by the mapper
callback (the new value is used directly without invoking
getProperty). Maybe I should log a bug to fix that, but
then that's probably not a big issue as it's returned
by the mapper.

However in the case at hand the private createLoggerHandler
method eventually does calls LogManager.getProperty(".handlers").
In the base LogManager that should be the only place where
LogManager.getProperty(".handlers") is called - I think.

Hmmm... I assume that might also depend on whether other classes
call org.apache.juli.ClassLoaderLogManager::getProperty(".handlers")
from outside, but then maybe that can be worked around in Tomcat
as well.

Let me try if I can get feedback on this suggestion.

best regards,

-- daniel

>
> Jason

Re: [JDK 11] RFR: 8195096: Exception printed on console with custom LogManager on starting Apache Tomcat

[Jason Mehrens]

Daniel,

Looking at: 
https://github.com/apache/tomcat/blob/trunk/java/org/apache/juli/ClassLoaderLogManager.java
 it looks like code in "readConfiguration(InputStream, ClassLoader)" does the 
digit translation.
The tomcat developers should be able to move some of that code to 
CLLM.getProperty to do the translation.

If they don't want JUL to create the root handlers then another option would be 
to alter CLLM.getProperty to determine if the j.ul.LogManager is trying to 
initialize ".handers" before CLLM.readConfiguration is called and return null 
to effectively disable creating handlers at that time.  My guess is  that this 
change is probably more compatible with what was already happening in Tomcat.

Jason


From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on behalf of 
Daniel Fuchs <daniel.fu...@oracle.com>
Sent: Friday, January 19, 2018 8:22 AM
To: core-libs-dev
Subject: Re: [JDK 11] RFR: 8195096: Exception printed on console with custom 
LogManager on starting Apache Tomcat

Hi,

For the record, I updated the JBS issue [1] with Jason's suggestion
and asked to get feedback from the submitter.

best regards,

-- daniel
[1] https://bugs.openjdk.java.net/browse/JDK-8195096


On 19/01/2018 10:13, Daniel Fuchs wrote:
> Hi Jason,
>
> On 18/01/2018 21:19, Jason Mehrens wrote:
>> Daniel,
>>
>> As long as the org.apache.juli.ClassLoaderLogManager overrides
>> getProperty it shouldn't really matter what the value format is in the
>> file as long as it is translated on return.
>> Is there a code path in j.u.l.LogManager that doesn't call
>> getProperty?  If so I would think that is the core issue.
>
> That's a very good question. Why didn't I think of it?
>
> AFAICS there's only one place where we do that, in the
> 'new' updateConfiguration method, and that's only for
> levels when a level value is changed by the mapper
> callback (the new value is used directly without invoking
> getProperty). Maybe I should log a bug to fix that, but
> then that's probably not a big issue as it's returned
> by the mapper.
>
> However in the case at hand the private createLoggerHandler
> method eventually does calls LogManager.getProperty(".handlers").
> In the base LogManager that should be the only place where
> LogManager.getProperty(".handlers") is called - I think.
>
> Hmmm... I assume that might also depend on whether other classes
> call org.apache.juli.ClassLoaderLogManager::getProperty(".handlers")
> from outside, but then maybe that can be worked around in Tomcat
> as well.
>
> Let me try if I can get feedback on this suggestion.
>
> best regards,
>
> -- daniel
>
>>
>> Jason
>

Re: [JDK 11] RFR: 8195096: Exception printed on console with custom LogManager on starting Apache Tomcat

[Jason Mehrens]

Daniel,

As long as the org.apache.juli.ClassLoaderLogManager overrides getProperty it 
shouldn't really matter what the value format is in the file as long as it is 
translated on return.
Is there a code path in j.u.l.LogManager that doesn't call getProperty?  If so 
I would think that is the core issue.

Jason

From: core-libs-dev  on behalf of 
Daniel Fuchs 
Sent: Thursday, January 18, 2018 9:12 AM
To: core-libs-dev
Subject: [JDK 11] RFR: 8195096: Exception printed on console with custom 
LogManager on starting Apache Tomcat

Hi,

Please find below a proposed fix for:

8195096: Exception printed on console with custom LogManager on
  starting Apache Tomcat
https://bugs.openjdk.java.net/browse/JDK-8195096

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8195096/webrev.00/

The issue appeared after the fix for
https://bugs.openjdk.java.net/browse/JDK-8191033
8191033: Regression in logging.properties: specifying .handlers=
  for root logger (instead of handlers=) no longer works

Tomcat is apparently reusing the ".handlers" property, to specify
class names in a format that the LogManager cannot understand
(hence the exceptions logged on the console).

The proposed fix is to add a protected hook that subclass of
LogManager could extend to turn off parsing of the ".handler"
property.

best regards,

-- daniel

Re: [10?] RFR: 8193128: Reduce number of implementation classes returned by List/Set/Map.of()

[Jason Mehrens]

What are your thoughts on pushing the static EMPTY_XXX declarations from 
ImmutableCollections down in to each respective inner class?  Doing that should 
allow for on demand class loading instead of getting everything when any 
factory is used.

>http://cr.openjdk.java.net/~redestad/8193128/open.04/

Jason

Re: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly set Exception

[Jason Mehrens]

Aleksei,

You have to override all of the constructors to always disable initCause.  
Actually a similar issue was covered in:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-May/016908.html
http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-June/017594.html 

Pretty much everything was hashed out in those threads.

Here is the final webrev for that:

http://cr.openjdk.java.net/~dmeetry/8009581/webrev.5/jaxp/src/javax/xml/xpath/XPathException.java.udiff.html

That should be a good cookbook for changes and tests required for this issue.  
Note that it gets rid of the Exception field and maintains serial 
compatibility.  
Looking back on that change, I would have changed it so the 
InvalidClassException added the double cause as suppressed exceptions.

Jason


From: core-libs-dev  on behalf of Aleks 
Efimov 
Sent: Thursday, December 21, 2017 11:27 AM
To: core-libs-dev
Subject: RFR [11] (JAXP): 6857903: SAXException.initCause() does not correctly 
set Exception

Hello,

Please, help to review the fix for jaxp bug that fixes SAXException to
correctly set exception cause with 'setCause' method:
http://cr.openjdk.java.net/~aefimov/6857903/dev/00/
I've tried to keep the fix miminal with respect to serial form of this
API class, i.e. kept private 'exception' field.

The new test case was added to IssueTracker56Test unit test. Testing
showed no related JCK/JTREG failures.

With Best Regards,
Aleksei

Re: RFR 4358774: Add null InputStream and OutputStream

[Jason Mehrens]

Brian,

My understand is JDK-6533165 is moving the bytes that are rarely invoked to a 
cold method.

Therefore:

if (closed) {
   throw new IOException("Stream closed");
}
==becomes===
if (closed) {
   throw sc();
}

private static IOException sc() {
return new IOException("Stream closed");
}


Since there is no string concatenation in the IOE message there are only a few 
bytes that can be shaved off.  I didn't benchmark it either case but I would 
assume it would matter for the nullOutputStream since the write methods could 
be invoked multiple times.

Jason

From: Brian Burkhalter <brian.burkhal...@oracle.com>
Sent: Wednesday, December 6, 2017 2:05 PM
To: Jason Mehrens
Cc: core-libs-dev
Subject: Re: RFR 4358774: Add null InputStream and OutputStream

Jason,

On Dec 6, 2017, at 11:54 AM, Jason Mehrens 
<jason_mehr...@hotmail.com<mailto:jason_mehr...@hotmail.com>> wrote:

For nullInputStream would it make any sense to use the ByteArrayInputStream 
with a (private static) empty byte array?  Maybe 'return new 
ByteArrayInputStream("".getBytes());'?  One side effect is that mark support 
returns true.

As we are trying to retain the behavior of closed streams throwing IOExceptions 
I don’t think that BAIS would work here.

Does it make sense to follow the advice 
inhttps://bugs.openjdk.java.net/browse/JDK-6533165 with regards to streams 
being closed?

I don’t know exactly what you intend here. In the linked issue there is 
information to impart, namely the index and the size. Here there is only the 
indication that the stream is closed. It’s not clear to me what could be done 
here.

Thanks,

Brian

Re: RFR 4358774: Add null InputStream and OutputStream

[Jason Mehrens]

Brian,

For nullInputStream would it make any sense to use the ByteArrayInputStream 
with a (private static) empty byte array?  Maybe 'return new 
ByteArrayInputStream("".getBytes());'?  One side effect is that mark support 
returns true.

Does it make sense to follow the advice in 
https://bugs.openjdk.java.net/browse/JDK-6533165 with regards to streams being 
closed?

Thanks,

Jason


From: core-libs-dev  on behalf of Brian 
Burkhalter 
Sent: Wednesday, December 6, 2017 1:00 PM
To: core-libs-dev
Subject: RFR 4358774: Add null InputStream and OutputStream

https://bugs.openjdk.java.net/browse/JDK-4358774
http://cr.openjdk.java.net/~bpb/4358774/webrev.00/

Add nullStream() method to each of InputStream and OutputStream.

Thanks,

Brian

Re: Change in properties for logging: deliberate?

[Jason Mehrens]

Hi Daniel,

Sorry for the late reply I was offline for the long weekend.

Hot reloads of the LogManager have always been a problem.  I think you are 
running into https://bugs.openjdk.java.net/browse/JDK-8033661 in your testing 
and that is going to give you troubling results on what is recreated after the 
call.  Make sure you test updateConfiguration which is the replacement everyone 
is to use going forward.

I think you'll want to make it so that "handlers" is just an alias name 
".handlers".  That way empty string is just name of the root logger which 
enables consistent use of other properties like ".level" and ".filter".
If both are defined in the logging.properties, then install the union of the 
two lines.

Jason


From: Daniel Fuchs <daniel.fu...@oracle.com>
Sent: Friday, November 10, 2017 10:04 AM
To: Jason Mehrens; mandy chung
Cc: core-libs-dev@openjdk.java.net
Subject: Re: Change in properties for logging: deliberate?

Hi Jason,

I have done a few tests with JDK 8 & 7.

I have created custom handlers and added some
debug traces in their constructors and debug methods.

Then I have added these two lines to my logging.properties:

handlers = custom.Handler
.handlers = custom.DotHandler

What I see is this:

- the first time the configuration is read, two handlers
   are added to the root logger:
   - an instance of DotHandler (first), then an instance
 of Handler (second).

Then if you call LogManager.readConfiguration() again,
both handlers are closed, and this time only one
instance of Handler is added to the root logger.
No instance of DotHandler is added.
 From now on the property is ignored.

This is because the root logger is a special beast:
it will not be removed (like all other loggers) when
LogManager.readConfiguration() is called.

And as it happens, handlers are added to loggers
when the loggers are added to the LogManager.
As it happens, the ".handlers" property is only parsed
and read when the root logger is added to the LogManager,
and thus only once.

The "handlers" property on the other hand is parsed
every time LogManager.readConfiguration() is called.

Given that, I suspect we should deprecate the use of
".handlers" for the root logger, as it appears that
it has never worked properly. I could work on a patch
for 10 (possibly backport it to 9 update) to preserve
the strange behavior of 7 & 8, but is it worth it?

What are your thoughts?

best regards,

-- daniel




On 09/11/2017 19:50, Jason Mehrens wrote:
> Daniel,
>
> I would assume you would fix since it is advertised as a feature over here: 
> https://docs.oracle.com/javase/1.5.0/docs/guide/logging/changes.html
>
> If it helps, I've dug up a lot of the history on this over here a while back:
> https://stackoverflow.com/questions/36726431/in-a-java-util-logging-logging-properties-file-whats-the-difference-between-h
>
> I've updated that to include the links to this new issue.  Now that I've 
> linked this message thread to that message thread that should crash the 
> internet. :)
>
> Jason
>
> 
> From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on behalf of 
> Daniel Fuchs <daniel.fu...@oracle.com>
> Sent: Thursday, November 9, 2017 1:29 PM
> To: mandy chung
> Cc: core-libs-dev@openjdk.java.net
> Subject: Re: Change in properties for logging: deliberate?
>
> On 09/11/2017 19:16, mandy chung wrote:
>> Daniel - we should add this known issue in the release note and document
>> the workaround.
>
> Hi Mandy,
>
> Right, it either need to be fixed, or documented in the release
> notes. Let me first have a look at the issue though.
>
> best regards,
>
> -- daniel
>

Re: Change in properties for logging: deliberate?

[Jason Mehrens]

Daniel,

I would assume you would fix since it is advertised as a feature over here: 
https://docs.oracle.com/javase/1.5.0/docs/guide/logging/changes.html

If it helps, I've dug up a lot of the history on this over here a while back:
https://stackoverflow.com/questions/36726431/in-a-java-util-logging-logging-properties-file-whats-the-difference-between-h

I've updated that to include the links to this new issue.  Now that I've linked 
this message thread to that message thread that should crash the internet. :)

Jason


From: core-libs-dev  on behalf of 
Daniel Fuchs 
Sent: Thursday, November 9, 2017 1:29 PM
To: mandy chung
Cc: core-libs-dev@openjdk.java.net
Subject: Re: Change in properties for logging: deliberate?

On 09/11/2017 19:16, mandy chung wrote:
> Daniel - we should add this known issue in the release note and document
> the workaround.

Hi Mandy,

Right, it either need to be fixed, or documented in the release
notes. Let me first have a look at the issue though.

best regards,

-- daniel

Re: 8189953: FileHandler constructor throws NoSuchFileException with absolute path

[Jason Mehrens]

Hi Daniel,

For more background on this issue I think you should link to this issue to the 
following:

https://bugs.openjdk.java.net/browse/JDK-8153250
https://bugs.openjdk.java.net/browse/JDK-8130462
https://bugs.openjdk.java.net/browse/JDK-8189862

Thanks,

Jason

From: core-libs-dev  on behalf of 
Daniel Fuchs 
Sent: Thursday, November 2, 2017 11:47 AM
To: core-libs-dev
Subject: RFR: 8189953: FileHandler constructor throws NoSuchFileException with 
absolute path

Hi,

Please find below a patch for:
8189953: FileHandler constructor throws NoSuchFileException
  with absolute path
https://bugs.openjdk.java.net/browse/JDK-8189953

webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8189953/webrev.00/

This is a windows only bug.

The issue is caused by a change that happen somewhen between
8 and 9, where new File(new File("C:"),"Workspace") was fixed
to return "C:Workspace" and not "C:\\Workspace".
This uncovered a bug in the FileHandler::generate algorithm.

The FileHandler::generate algorithm could arguably be improved
by being entirely rewritten but I choose to keep the changes
as minimal as I could.

best regards,

-- daniel

Re: ThreadPoolExecutor and finalization

[Jason Mehrens]

Peter,

Executors.unconfigurableExecutorService was not modified on purpose because of 
the following case:  
http://cs.oswego.edu/pipermail/concurrency-interest/2006-March/002353.html

Jason


From: core-libs-dev  on behalf of Peter 
Levart 
Sent: Thursday, November 2, 2017 10:23 AM
To: David Holmes; Roger Riggs
Cc: core-libs-dev
Subject: Re: ThreadPoolExecutor and finalization

Hi,

On 11/02/2017 01:47 PM, David Holmes wrote:
>> public class CleanableExecutorService implements ExecutorService {
>>  private final ThreadPoolExecutor tpe;
>>
>>  public CleanableExecutorService(ThreadPoolExecutor tpe) {
>>  CleanerFactory.cleaner().register(this, tpe::shutdown);
>>  this.tpe = tpe;
>>  }
>>
>>  // implement and delegate all ExecutorService methods to tpe...
>> }
>
> Ah I see - the old "extra level of indirection" solution. :) The
> Cleaner keeps the tpe strongly reachable, but as soon as the holder
> class becomes "unreachable" the Cleaner will shutdown the tpe.

  I see there already is the following method in Executors:

 public static ExecutorService newSingleThreadExecutor() {
 return new FinalizableDelegatedExecutorService
 (new ThreadPoolExecutor(1, 1,
 0L, TimeUnit.MILLISECONDS,
 new LinkedBlockingQueue()));
 }

 private static class FinalizableDelegatedExecutorService
 extends DelegatedExecutorService {
 FinalizableDelegatedExecutorService(ExecutorService executor) {
 super(executor);
 }
 @SuppressWarnings("deprecation")
 protected void finalize() {
 super.shutdown();
 }
 }


If the same FinalizableDelegatedExecutorService was used also for the
following method:

 /**
  * Returns an object that delegates all defined {@link
  * ExecutorService} methods to the given executor, but not any
  * other methods that might otherwise be accessible using
  * casts. This provides a way to safely "freeze" configuration and
  * disallow tuning of a given concrete implementation.
  * @param executor the underlying implementation
  * @return an {@code ExecutorService} instance
  * @throws NullPointerException if executor null
  */
 public static ExecutorService
unconfigurableExecutorService(ExecutorService executor) {
 if (executor == null)
 throw new NullPointerException();
 return new DelegatedExecutorService(executor);
 }


...we would get such ExecutorService out of the box.


Regards, Peter

Re: ThreadPoolExecutor and finalization

[Jason Mehrens]

Hi Peter,

Most of the discussion is in: https://bugs.openjdk.java.net/browse/JDK-6399443. 
 The linked issue in that report then links to the CI mail thread.

Jason


>I'm trying to understand the purpose of finalize() in TPE, but can't.
>I'm surely missing something. If the pool is no longer referenced AND
>there are no active threads, what is there left to shutdown() actually?
>All that remains is garbage that will eventually be GCed.

>Regards, Peter

Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator

[Jason Mehrens]

Makes sense on the sub-sequences having to be string.  Looks good.

Jason


From: Ivan Gerasimov <ivan.gerasi...@oracle.com>
Sent: Thursday, September 28, 2017 3:19 PM
To: Jason Mehrens; core-libs-dev
Subject: Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator

Thank you Jason for valuable input!


On 9/28/17 7:22 AM, Jason Mehrens wrote:
> Ivan,
>
> Am I correct that subtraction of code points is safe from underflow due to 
> the range of Character.MIN_CODE_POINT and Character.MAX_CODE_POINT?  That 
> would explain why you are not using Integer.compare.
Right.  Since codepoints are all positive, it should be safe to subtract
them.

> One alternative to calling CharSequence.subSequence is to use 
> CharBuffer.wrap(CharSequence csq, int start, int end).  The sub-sequences are 
> short lived so that may be faster in the String case.
Yes, it's a good point, given how expensive String.subSequence() is.
The disadvantage of using CharBuffer.wrap() is that it wouldn't be
possible to use the standard String comparators as the custom
alphaComparator.
With the current implementation, it is possible (with additional casts,
of course) because we know that String.subSequence returns a String object.

To be honest, I see no easy way to optimize the String case with the use
of a custom comparator because we need to pass to it portions of the
input as String objects.
One possibility might be to recognize common cases, like using the
standard comparators (for example, String.CASE_INSENSITIVE_ORDER) in
place of the custom alphaComparator, and then execute special variant of
the routine, which doesn't create explicit subsequences, but works on
the entire sequence instead.

At this point, however, it's probably too early to do such optimizations.

> Admittedly this is more of a Claes Redestad faster boot up type change but, 
> the use of the AlphaDecimalComparator constructor in Comparator might force 
> the bytecode verifier to force load AlphaDecimalComparator just to check the 
> return type.
Hm, interesting.
We can introduce another factory method in the Comparators class, so
that AlphaDecimalComparator won't be mentioned in the Comparator at all.

Please see the updated webrev:
http://cr.openjdk.java.net/~igerasim/8134512/05/webrev/

With kind regards,
Ivan


> If you make factory methods in AlphaDecimalComparator you can usually craft 
> the return type to match and the verify of Comparator will not force load the 
> AlphaDecimalComparator.
> Otherwise, the more ugly solution is to wrap 'new' with Comparator.class.cast.
>
> Jason
>
>
>
> 
> From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on behalf of 
> Ivan Gerasimov <ivan.gerasi...@oracle.com>
> Sent: Monday, September 25, 2017 12:49 PM
> To: core-libs-dev
> Subject: Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator
>
> Hello!
>
> Could you please review at your convenience?
>
> In the latest webrev I took all suggestions into account (unless I
> missed something.)
>
> http://cr.openjdk.java.net/~igerasim/8134512/04/webrev/
>
>
> I think, if the suggested comparator is found useful by the users, then
> it may make sense to create the String-oriented variant, which can be
> implemented through the CharSequence-oriented one as:
>
> class String {
>   ...
>   @SuppressWarnings("unchecked")
>   public static  Comparator
>   comparingAlphaDecimal(Comparator alphaComparator) {
>   return (Comparator) (Comparator)
>   new
> Comparators.AlphaDecimalComparator<>(Objects.requireNonNull(
>   (Comparator) alphaComparator),
> false);
>   }
> }
>
> This will be safe, since the specification guarantees that
> String.subSequence() returns a String.
>
> Then in the application code it would be possible to instantiate the
> comparators as
>
>   String.comparingAlphaDecimal(String::compareTo);
>
>   String.comparingAlphaDecimal(String::compareToIgnoreCase);
>
> or, alternatively,
>   String.comparingAlphaDecimal(Comparator.naturalOrder());
>
> String.comparingAlphaDecimal(String.CASE_INSENSITIVE_ORDER);
>
> But this could be deferred for later, of course.
>
> With kind regards,
> Ivan
>
>
> On 8/27/17 1:38 PM, Ivan Gerasimov wrote:
>> Hello everyone!
>>
>> Here's another iteration of the comparator with suggested improvements.
>>
>> Now, there is the only input argument -- the alpha-comparator for
>> comparing the non-decimal-digit sub-sequences.
>>
>> For the javadoc I used the text suggested by Peter with some
>> modifications, additional example and API/imp

Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator

[Jason Mehrens]

Ivan,

Am I correct that subtraction of code points is safe from underflow due to the 
range of Character.MIN_CODE_POINT and Character.MAX_CODE_POINT?  That would 
explain why you are not using Integer.compare.

One alternative to calling CharSequence.subSequence is to use 
CharBuffer.wrap(CharSequence csq, int start, int end).  The sub-sequences are 
short lived so that may be faster in the String case.

Admittedly this is more of a Claes Redestad faster boot up type change but, the 
use of the AlphaDecimalComparator constructor in Comparator might force the 
bytecode verifier to force load AlphaDecimalComparator just to check the return 
type.

If you make factory methods in AlphaDecimalComparator you can usually craft the 
return type to match and the verify of Comparator will not force load the 
AlphaDecimalComparator.
Otherwise, the more ugly solution is to wrap 'new' with Comparator.class.cast.

Jason




From: core-libs-dev  on behalf of Ivan 
Gerasimov 
Sent: Monday, September 25, 2017 12:49 PM
To: core-libs-dev
Subject: Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator

Hello!

Could you please review at your convenience?

In the latest webrev I took all suggestions into account (unless I
missed something.)

http://cr.openjdk.java.net/~igerasim/8134512/04/webrev/


I think, if the suggested comparator is found useful by the users, then
it may make sense to create the String-oriented variant, which can be
implemented through the CharSequence-oriented one as:

class String {
 ...
 @SuppressWarnings("unchecked")
 public static  Comparator
 comparingAlphaDecimal(Comparator alphaComparator) {
 return (Comparator) (Comparator)
 new
Comparators.AlphaDecimalComparator<>(Objects.requireNonNull(
 (Comparator) alphaComparator),
false);
 }
}

This will be safe, since the specification guarantees that
String.subSequence() returns a String.

Then in the application code it would be possible to instantiate the
comparators as

 String.comparingAlphaDecimal(String::compareTo);

 String.comparingAlphaDecimal(String::compareToIgnoreCase);

or, alternatively,
 String.comparingAlphaDecimal(Comparator.naturalOrder());

String.comparingAlphaDecimal(String.CASE_INSENSITIVE_ORDER);

But this could be deferred for later, of course.

With kind regards,
Ivan


On 8/27/17 1:38 PM, Ivan Gerasimov wrote:
> Hello everyone!
>
> Here's another iteration of the comparator with suggested improvements.
>
> Now, there is the only input argument -- the alpha-comparator for
> comparing the non-decimal-digit sub-sequences.
>
> For the javadoc I used the text suggested by Peter with some
> modifications, additional example and API/implementation notes.
> Overall, the javadoc looks heavier than need to me, so I'd love to
> hear comments about how to make it shorter and cleaner.
>
> Also, I adopted the name AlphaDecimal, suggested by Peter.  This name
> is one of popular in the list of variants found in the wild. So, there
> are higher chances the users can find the routine by its name.
>
> For testing if a code point is a decimal digit, I used
> (Character.getType(cp) == Character.DECIMAL_DIGIT_NUMBER), which seem
> to be more appropriate than Character.isDigit().  (The later is true
> for things like a digit in a circle, superscript, etc., which do not
> seem to be a part of a decimal number composed of several digits.)
>
> The updated webrev:
> http://cr.openjdk.java.net/~igerasim/8134512/04/webrev/
>
> Please review at your convenience.
>
> With kind regards,
> Ivan
>
> On 8/9/17 4:59 PM, Stuart Marks wrote:
>> On 8/1/17 11:56 PM, Ivan Gerasimov wrote:
>>> I've tried to go one step further and created even more abstract
>>> comparator:  It
>>> uses a supplied predicate to decompose the input sequences into
>>> odd/even
>>> subsequences (e.g. alpha/numeric) and then uses two separate
>>> comparator to
>>> compare them. Additionally, a comparator for comparing sequences,
>>> consisting
>>> only of digits is provided. For example, to build a case-insensitive
>>> AlphaDecimal comparator one could use: 1) Character::isDigit -- as
>>> the predicate
>>> for decomposing, 2) String::compareToIgnoreCase -- to compare alpha
>>> (i.e. odd
>>> parts); to work with CharSequences one would need to make it
>>> Comparator.comparing(CharSequence::toString,
>>> String::compareToIgnoreCase), 3)
>>> The special decimal-only comparator, which compares the decimal
>>> representation
>>> of the sequences. Here's the file with all the comparators and a
>>> simple test:
>>> http://cr.openjdk.java.net/~igerasim/8134512/test/Test.java
>>
>> Hi, a couple follow-up thoughts on this.
>>
>> 1) Supplementary characters
>>
>> The current code uses Character.isDigit(char), which works only for
>> char values in the BMP (basic multilingual plane, values <= 

Re: JDK java.util.concurrent.ConcurrentSkipListSet.equals(Object o) implementation efficiency

[Jason Mehrens]

Hi Doug,

>However, this technique might apply more usefully to TreeSet.

True.  The more I think about it, it is probably applicable to CSLS under the 
context where it is concurrently added to and then reaches some idle state 
(shutdown) and then performs the equality test.  It could be port it to CSLM 
and set views too.

Jason

Re: [REVISED] JDK 9 doc-api-only RFR of 6791812: (file spec) Incompatible File.lastModified() and setLastModified() for negative time

[Jason Mehrens]

Brian,

Is there a dangling '}' after Files.readAttributes or missing '{@link '?

Jason


From: core-libs-dev  on behalf of Brian 
Burkhalter 
Sent: Friday, May 26, 2017 11:17 AM
To: Stuart Marks
Cc: core-libs-dev
Subject: Re: [REVISED] JDK 9 doc-api-only RFR of 6791812: (file spec) 
Incompatible File.lastModified() and setLastModified() for negative time

Here’s one more attempt:

http://cr.openjdk.java.net/~bpb/6791812/webrev.01/
http://cr.openjdk.java.net/~bpb/6791812/File.html

Similar verbal de-obfuscation of other methods may be handled under other 
yet-to-be-file issues.

Thanks,

Brian

Re: [9] RFR: 8177136: Caller sensitive method System.getLogger should specify what happens if there is no caller on the stack.

[Jason Mehrens]

Hi Daniel,

For my own understanding, would it be possible and yet still secure to walk up 
the call stack to substitute a caller in the case of method being defined with 
the 'native' keyword assuming it is in the same class?
For instance, if you look at the stacktrace in JDK-8145302 there is a native 
method calling 'getLogger' with a signature defined in class called MWMCR.  
Then are 2 frames from the MWMCR class and 2 frames from what appears to be 
anonymous inner classes of MWMCR.

If all we care about is the calling class it seems like there are enough frames 
to choose from it just that they are not between 'getLogger' and the native 
method.

Jason


From: core-libs-dev  on behalf of 
Daniel Fuchs 
Sent: Wednesday, March 22, 2017 1:29 PM
To: Mandy Chung
Cc: core-libs-dev
Subject: Re: [9] RFR: 8177136: Caller sensitive method System.getLogger should 
specify what happens if there is no caller on the stack.

On 22/03/2017 18:20, Mandy Chung wrote:
>
>> On Mar 22, 2017, at 10:34 AM, Daniel Fuchs  wrote:
>>
>> http://cr.openjdk.java.net/~dfuchs/webrev_8177136/webrev.04
>
> Looks okay.
>
> "an {@code IllegalCallerException} is thrown.”
>- It might read better if you drop “an” and add a comma instead
>
> No need to have a new webrev.

Thanks Mandy,

I updated the webrev in place. I will now proceed with all
the paperwork to get that fix in 9 ;-)

http://cr.openjdk.java.net/~dfuchs/webrev_8177136/webrev.04

-- daniel

>
> Thanks
> Mandy
>

Re: Request for Review and Sponsor needed: JDK-8167648: java.io.PrintWriter should have PrintWriter((String|File), Charset) constructors

[Jason Mehrens]

You should add a subclass test to prove that it works in practice.  If the 
subclasses are required to override the method then shouldn't the 
'withAutoFlush' method check that getClass() == PrintWriter.class otherwise 
throw something like UnsupportedOperationException?

Jason


From: Patrick Reinhart <patr...@reini.net>
Sent: Tuesday, December 27, 2016 8:46 AM
To: Jason Mehrens
Cc: core-libs-dev
Subject: Re: Request for Review and Sponsor needed: JDK-8167648: 
java.io.PrintWriter should have PrintWriter((String|File), Charset) constructors

Hi Jason,

At the moment, a subclass would need to overwrite this method with the
same behaviour. The other solution would be to make the internal state
auto-flush to no longer be final.

-Patrick

On 2016-12-21 22:40, Jason Mehrens wrote:
> Patrick,
>
> How is 'withAutoFlush' expected to behave for subclasses of
> PrintWriter?
>
> Jason

Re: Request for Review and Sponsor needed: JDK-8167648: java.io.PrintWriter should have PrintWriter((String|File), Charset) constructors

[Jason Mehrens]

Patrick,

How is 'withAutoFlush' expected to behave for subclasses of PrintWriter?

Jason


From: core-libs-dev  on behalf of 
Patrick Reinhart 
Sent: Wednesday, December 21, 2016 3:08 PM
To: Roger Riggs
Cc: core-libs-dev
Subject: Re: Request for Review and Sponsor needed: JDK-8167648:
java.io.PrintWriter should have PrintWriter((String|File), Charset) 
constructors

Hi Roger,

I tried to put your suggested changes, into the following webrev:

http://cr.openjdk.java.net/~reinhapa/reviews/8167648/webrev.01


 - 375:  Can this use the new private constructor that will handle psOut.
>>> Here I can not get hold on the encoding at this point or have I missed 
>>> something here?
>> True, not easily, it is available as a String from 
>> OutputStreamWriter.getEncoding() but it would suffer from
>> having to lookup it by name again.
>
> Right, even the Charset is actually available within the StreamEncoder 
> implementation but not provided to the outside world.
>
> Also the OutputStreamWriter is in wrapped in a BufferedWriter and would be 
> needed to be extracted from there again in the first place.

Here I left the invocation as it was before…

-Patrick

Re: RFR 8170348: Appendable.appendN(char, int) method to append multiple copies of char

[Jason Mehrens]

Ivan,

Will java.util.StringJoiner be modified too?  I assume a nCopies char sequence 
object doesn't pan out performance wise?

Thanks,

Jason


From: core-libs-dev  on behalf of Ivan 
Gerasimov 
Sent: Sunday, December 4, 2016 6:07 AM
To: core-libs-dev@openjdk.java.net
Subject: RFR 8170348: Appendable.appendN(char, int) method to append multiple   
copies of char

Hello!

There are several places in JDK where the same character is appended to
a StringBuilder object multiple times (usually padding).
With each append there are a few routine checks performed.
They could have been done only once, if we had a method for appending
multiple copies at a time.
A simple benchmark shows that such method may save us a few machine
cycles (see the results below).

In the benchmark, three approaches were compared:
0) Using the new appendN(char, int) method to append several chars at once,
1) Calling append(char) in a loop,
2) Appending a prepared-in-advance string

On my machine, the new method demonstrates better or comparable
performance for all sizes up to 20.

In the webrev, there are two changesets included:
- the new default Appendable.appendN(char, int) method, its overrides in
StringBuilder/Buffer and a basic test,
- several applications of the new method across JDK.

Would you please help review?
Comments, suggestions are welcome.

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8170348
WEBREV: http://cr.openjdk.java.net/~igerasim/8170348/00/webrev/
Benchmark: http://cr.openjdk.java.net/~igerasim/8170348/00/MyBenchmark.java


Benchmark (size)   Mode  Cnt Score  Error  Units
MyBenchmark.test_0_New 0  thrpt   70  331922128.215 ±
16399254.452  ops/s
MyBenchmark.test_0_New 1  thrpt   70  209207932.893 ±
14955800.231  ops/s
MyBenchmark.test_0_New 5  thrpt   70   72926671.621 ±
4841791.555  ops/s
MyBenchmark.test_0_New10  thrpt   70   67779575.053 ±
3234366.239  ops/s
MyBenchmark.test_0_New20  thrpt   70   59731629.661 ±
2769497.288  ops/s
MyBenchmark.test_1_Old 0  thrpt   70  333467628.860 ±
15981678.430  ops/s
MyBenchmark.test_1_Old 1  thrpt   70  156126381.967 ±
9619653.294  ops/s
MyBenchmark.test_1_Old 5  thrpt   70   46550204.382 ±
2009987.637  ops/s
MyBenchmark.test_1_Old10  thrpt   70   23309297.849 ±
1268874.282  ops/s
MyBenchmark.test_1_Old20  thrpt   70   13143637.821 ±
662265.103  ops/s
MyBenchmark.test_2_Solid   0  thrpt   70  138548108.540 ±
6408775.462  ops/s
MyBenchmark.test_2_Solid   1  thrpt   70   63890936.132 ±
3918274.970  ops/s
MyBenchmark.test_2_Solid   5  thrpt   70   65838879.075 ±
2701493.698  ops/s
MyBenchmark.test_2_Solid  10  thrpt   70   65387238.993 ±
3131562.548  ops/s
MyBenchmark.test_2_Solid  20  thrpt   70   57528150.828 ±
3171453.716  ops/s


With kind regards,
Ivan

Re: RFR: 8152515: (logging) LogManager.resetLogger should ignore LinkageError

[Jason Mehrens]

The only other fix would be to collect all errors thrown (using addSuppressed) 
during close and throw only after the we have attempted to close all handlers.  
Then we would retain the behavior of always throwing errors.

Jason


From: Daniel Fuchs <daniel.fu...@oracle.com>
Sent: Tuesday, November 01, 2016 5:43 AM
To: David Holmes; core-libs-dev
Cc: Jason Mehrens
Subject: Re: RFR: 8152515: (logging) LogManager.resetLogger should ignore 
LinkageError

On 28/10/16 20:32, David Holmes wrote:
> Hi Daniel,
>
> I've read the bug report on this and this issue "smells" to me.
> LinkageError should not be a special case IMHO. The existing code was
> trying to go the "ignore everything but 'serious errors' " route - but
> without really considering what constitutes a "serious error". I think
> the fact a LinkageError can turn up here reflects a bug in the code that
> throws it - or possibly in something in between.
>
> There should be a very clear exception handling policy for this code,
> and not special cases IMHO. Having that policy depend on whether
> shutdown is in progress is okay to me as we know that things can behave
> unexpectedly in that case. So I would advocate for a more general
>
> + } catch (Throwable e) {
> + // Ignore all exceptions while shutting down
> + if (globalHandlersState != STATE_SHUTDOWN) {
> + throw e;
> + }
>

Hi David,

This is something I had been considering too.

In the end it made me feel uneasy to trap all errors - like OOME
or IllegalAccessError (swallowing IAE is not great when
one migrates from JDK 8 to JDK 9 where IAE helps diagnose
encapsulation issues) - so I opted for the minimal fix.

I agree that just trapping LinkageError is a bit strange,
but we do have a use case for that - which AFAIU is not
uncommon.

Anyway, if you ask me then the root of the problem is
that the various shutdown hook can run in parallel and in
unspecified order - so making the reset() more robust by
trapping all errors might not be such a bad idea after all...
Here is an updated webrev:

http://cr.openjdk.java.net/~dfuchs/webrev_8152515/webrev.02/


  } catch (Exception ex) {
  // Problems closing a handler?  Keep going...
+} catch (Error e) {
+// ignore Errors while shutting down
+if (globalHandlersState != STATE_SHUTDOWN) {
+throw e;
+}
  }

best regards,

-- daniel


> Cheers,
> David
>
> On 28/10/2016 9:51 PM, Daniel Fuchs wrote:
>> Hi,
>>
>> Please find below a trivial  patch for:
>>
>> 8152515: (logging) LogManager.resetLogger should ignore LinkageError
>> https://bugs.openjdk.java.net/browse/JDK-8152515
>>
>>
>> Patch:
>> http://cr.openjdk.java.net/~dfuchs/webrev_8152515/webrev.00/
>>
>> The issue might occur at shutdown, when a handler that makes uses
>> of some APIs provided by an OSGI bundle which was already closed
>> by the shutdown process is in turn closed by the LogManager.Cleaner
>> thread. In that case some subclasses of LinkageError may be thrown,
>> interrupting the reset process and preventing other handlers from
>> being closed properly.
>>
>> The patch proposes to trivially ignore LinkageError at shutdown while
>> the LogManager.Cleaner thread is running.
>>
>> best regards,
>>
>> -- daniel

Re: 8152515: (logging) LogManager.resetLogger should ignore LinkageError

[Jason Mehrens]

Daniel,

Looks good to me.

Thanks for fixing this!

Jason


From: Daniel Fuchs <daniel.fu...@oracle.com>
Sent: Friday, October 28, 2016 6:51 AM
To: core-libs-dev
Cc: Jason Mehrens
Subject: RFR: 8152515: (logging) LogManager.resetLogger should ignore 
LinkageError

Hi,

Please find below a trivial  patch for:

8152515: (logging) LogManager.resetLogger should ignore LinkageError
https://bugs.openjdk.java.net/browse/JDK-8152515


Patch:
http://cr.openjdk.java.net/~dfuchs/webrev_8152515/webrev.00/

The issue might occur at shutdown, when a handler that makes uses
of some APIs provided by an OSGI bundle which was already closed
by the shutdown process is in turn closed by the LogManager.Cleaner
thread. In that case some subclasses of LinkageError may be thrown,
interrupting the reset process and preventing other handlers from
being closed properly.

The patch proposes to trivially ignore LinkageError at shutdown while
the LogManager.Cleaner thread is running.

best regards,

-- daniel

Re: 8153666: Optimize Formatter.formatMessage

[Jason Mehrens]

Hi Daniel,

Thanks for taking this on.  Looks good.

This might be a new issue altogether but, if record.getMessage returns null a 
NPE can be generated during 'catalog.getString' (per RB.handleGetObject 
contract)  or 'java.text.MessageFormat.format' (undocumented)
It is handled by the catch clause so it is harmless in terms of correctness.  
The most common form of this abuse I've seen in the wild is
'logger.log(SEVERE, null, (Throwable) ioe);

So if it not worth explicitly checking for null maybe we should add a comment 
explaining the intent that NPE could be generated and is trapped.
For sure having tests for null message with and without a resource bundle would 
be a good idea.

Thanks,

Jason


From: core-libs-dev  on behalf of 
Daniel Fuchs 
Sent: Wednesday, June 8, 2016 8:46 AM
To: core-libs-dev
Subject: RFR: 8153666: Optimize Formatter.formatMessage
    
Hi,

Please find below a patch for a small optimization
in Formatter.formatMessage.
This patch also removed the synchronized keyword which
was there for historical reasons - but which has
become needless.

More background available at:
https://bugs.openjdk.java.net/browse/JDK-8153666
(thanks Martin!)
and
http://stackoverflow.com/questions/36433146/why-is-the-formatmessage-method-in-java-util-logging-formatter-synchronized
(thanks Jason!)

bug:
8153666: Optimize Formatter.formatMessage
https://bugs.openjdk.java.net/browse/JDK-8153666


patch:
http://cr.openjdk.java.net/~dfuchs/webrev_8153666/webrev.00

best regards, and thanks to all who provided suggestions and
archeological background!

-- daniel

Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival

[Jason Mehrens]

Brent,

Have you considered increasing the visibility of just the EntrySet constructor 
from private to default access?  This should get rid an extra generated class 
file if you compare the javac output since you are accessing just the 
constructor from the enclosing class.

Jason


From: core-libs-dev  on behalf of Brent 
Christian 
Sent: Tuesday, May 17, 2016 8:46 PM
To: Mandy Chung; David Holmes
Cc: core-libs-dev
Subject: Re: RFR 8029891 : Deadlock detected in 
java/lang/ClassLoader/deadlock/GetResource.java - A Revival

Hi!

Here's the final(?) webrev:
   http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/webrev/
with the following changes since the last one:

* The @hidden JavaDoc tag has been removed.  It can't be used due to
methods disappearing from the API page.  Living with the additional
JavaDoc is the best option for now.  The situation should be improved by
JDK-8157000 (thanks for filing, Mandy).

* I also firmed up the doc update regarding iterators (using words from
the java.util.concurrent package description of "weakly-consistent").
Line 170.

New specdiff views:

http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/specdiff-javadoc/Properties.html

http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/specdiff-plain/Properties.html

* I did make one real code change, making the new Properties.EntrySet
inner class static.  Line 1250.

* I added a new test of unsynchronized, as suggested by Peter, and
expanded coverage to all newly-unsynchronized methods.

Thanks,
-Brent

On 5/13/16 8:01 AM, Mandy Chung wrote:
>
>> On May 12, 2016, at 5:58 PM, David Holmes  wrote:
>>>
>>> With all of the inherited methods @hidden, it looks like that section
>>> is left out altogether.
>>
>> Okay, so I have to say @hidden seems to me to be seriously flawed! If a 
>> class has a method that can be called then IMHO that has to be documented in 
>> that class as either being first defined, overridden, or inherited - it 
>> can't just say nothing as-if the method were not there!
>>
>> If we are overriding in a trivial way that does not change the specification 
>> at all then there should be a simple way to show that - perhaps the "Methods 
>> inherited from ..." should be split into two parts: method implementations 
>> inherited from ..., and method specifications inherited from ... ??
>>
>
> I agree for this case these methods should not be hidden as if it’s not there 
> (that’s probably carried from the original @treatAsPrivate request).
>
>> I guess I need to raise this with the javadoc folk :( Is there a mailing 
>> list for that?
>
> Can you file a JBS issue?  Kumar is on vacation and will talk with him when 
> he’s back.
>
> Mandy
>

Re: JDK 9 pre-review of JDK-6850612: Deprecate Class.newInstance since it violates the checked exception language contract

[Jason Mehrens]

Hi Peter,

>As mentioned, Class.newInstance() has a special cache for constructor
>and caller that speeds up repeated invocations from the same caller by
>skipping access checks.

I'm sure I'm missing something obvious related to performance or security but, 
couldn't the exact same 'cachedConstructor' field be used for caching  
Class.getConstructor(new Class[0]) as long as a constructor copy is returned?
That way the recommended workaround is near the same performance.

Jason

Re: 8152436: Add a test to verify that the root logger correctly reports the caller's information

[Jason Mehrens]

Understood.  When I tested a subclassed logger all of the JDKs work correctly.  
I didn't check but it just seems like something that could regress if there is 
no test.

Regards,

Jason


From: Daniel Fuchs <daniel.fu...@oracle.com>
Sent: Tuesday, March 22, 2016 11:29 AM
To: Jason Mehrens; core-libs-dev
Subject: Re: 8152436: Add a test to verify that the root logger correctly 
reports the caller's information

On 22/03/16 17:11, Jason Mehrens wrote:
> Hi Daniel,
>
> I think we just need to add a test where a subclass logger logs the message 
> with the root logger as the parent.  Should be no need to register that 
> logger with the LogManager.
> Not sure if it is worth checking that custom filters installed on handlers or 
> logger and or formatters fool anything.

Hi Jason,

That would be another issue.

I believe the current behavior in 9 is that the custom logger
subclass will appear as the emitter of the log message, unless
it also implements System.Logger.

Now that we use the StackWalker API we have the opportunity
to use Class.isAssignableFrom to do the filtering - so we
could fix LogRecord::inferLogger to skip subclasses of
java.util.logging.Logger as well - which we can't do in 8
(well we could use Reflection.getCallerClass(int) in 8, but
that would not be a great idea - so I'd prefer to keep that as
a limitation for 8).

The test below is mainly to verify JDK-8152389 - which is about
verifying that calling Logger.getLogger("").() does
not report LogManager$RootLogger as the calling frame.

I should probably however log an RFE against 9 to skip
custom subclasses of java.util.logging.Logger when
inferring caller information. And then I'll just have to
extend the new test to also test this new scenario.

best regards,

-- daniel

>
> Regards,
>
> Jason
>
> 
> From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on behalf of 
> Daniel Fuchs <daniel.fu...@oracle.com>
> Sent: Tuesday, March 22, 2016 10:03 AM
> To: core-libs-dev
> Subject: RFR: 8152436: Add a test to verify that the root logger correctly
>   reports the caller's information
>
> Hi,
>
> Please find below a new test that verifies that JDK-8152389 does
> not occur in JDK 9.
>
> bug:
> 8152436: Add a test to verify that the root logger correctly
>reports the caller's information
> https://bugs.openjdk.java.net/browse/JDK-8152436
>
> Issue being verified by the test:
> https://bugs.openjdk.java.net/browse/JDK-8152389
>
> Webrev (test only):
> http://cr.openjdk.java.net/~dfuchs/webrev_8152436/webrev.00/
>
> best regards,
>
> -- daniel
>

Re: RFR: 8145680: Remove unnecessary explicit initialization of volatile variables in java.base

[Jason Mehrens]

So for C2 it doesn't matter.  From what I can tell using javap, the xor 
generates the fewest number of bytecode operations.  Not that this breaks the 
bank but, one would think that fewer bytecodes would help C1.

Jason

From: core-libs-dev  on behalf of 
Aleksey Shipilev 
Sent: Monday, December 21, 2015 4:02 AM
To: Andrew Haley; core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8145680: Remove unnecessary explicit initialization of
volatile variables in java.base

On 12/21/2015 12:46 PM, Andrew Haley wrote:
>> See: "Speed-kings of inverting booleans" at
>> http://www.javaspecialists.eu/archive/Issue042.html
>
> That's from 2002, and not valid any more.  Maybe not valid even back
> then.

It is not valid today, I've checked a while ago:
 http://cr.openjdk.java.net/~shade/scratch/BooleanNegate.java

-Aleksey

Re: RFR: 8145680: Remove unnecessary explicit initialization of volatile variables in java.base

[Jason Mehrens]

Claes,

For the cases where boolean was being assigned to 'true' (ASSCI and 
FileLockImpl) does it hurt performance since the accessor methods will now 
include a branch at the bytecode level?  See: "Speed-kings of inverting 
booleans" at  http://www.javaspecialists.eu/archive/Issue042.html

Jason

From: core-libs-dev  on behalf of Claes 
Redestad 
Sent: Sunday, December 20, 2015 11:29 AM
To: core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8145680: Remove unnecessary explicit initialization of
volatile variables in java.base

Hi,

the changes to java.net.URI stood out as a bit too intrusive for a
cleanup like this, and there's precious little measurable benefit. I
decided to break out those to a separate RFE and simplified this patch:

Webrev: http://cr.openjdk.java.net/~redestad/8145680/webrev.02

/Claes

On 2015-12-19 14:07, Claes Redestad wrote:
> Hi,
>
> initializing volatile fields to their default value has a well-known
> performance penalty, and removing these should typically be safe. This
> patch addresses java.base.
>
> There are however some corner cases that we need to check for, see
> examples and discussion in
> http://cs.oswego.edu/pipermail/concurrency-interest/2015-December/014767.html
>
> When meticulously going through and checking each usage for odd
> pattern like this I accidentally did a bit of extra cleanup, mostly
> addressing a number of cases where the volatile field was being read
> twice. Sorry!
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8145680
>

Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions

[Jason Mehrens]

Alexander,

>What is wrong with get/serErrorManager for Formatter? It should be just
>a way for Formatter to report its internal errors.

1. You can't inherit the Handler error manager until after construction of the 
formatter.  That means the concept of inheriting an error manager from a 
handler is flawed.  Forcing subclasses to include an ErrorManager or Handler 
constructor is not very clean either.  Especially if the formatter already has 
constructors that take multiple arguments. 

2.  Formatters can decorate other formatters.  See point #1.  Logging does not 
allow for creating an ErrorManager that is hostile that allows an exception to 
escape (without resorting to hacks).  Making the internal and external 
formatter work as one unit gets messy.  

3. How do you control System.err chatter?  It used to be that every handler had 
an error manager and every error manager could report one problem.  If we 
increase the number of error managers we can increase the amount of chatter.

3. The JDK Handlers already expect that formatters throw exceptions see 
(StreamHandler).  Publishing is where things are expected to fail.  Everything 
that happens upstream from a Handler is expected not to fail.  It is already 
cleanly established that the handler should be the mediator between the 
formatter and the error manager.  The creation of a handler creates the error 
manager and formatters so it is much cleaner to have the formatter simply throw 
exceptions to the handler and have the handler delegate to the error manager.

4. ErrorManagers installed on formatters would mainly deal with programing bugs 
(pattern syntax errors and broken Object.toString implementations) not expected 
failures like I/O.  Seems excessive to add an ErrorManager that conditionally 
handles bugs.  Handers on the other hand can fail with checked exceptions like 
I/O which is a different class of errors.

5. ErrorManager properties on a Formatter is more API complexity and more 
mutable state to deal with and test.  I think API producers and API consumers 
would like to avoid this debt.

6. ErrorManager defines an error code as an argument.  If objects belonging to 
a Handler were supposed to have an ErrorManager we wouldn't need the code 
argument.  This new addtion conflicts with the original design.

7. It is wasted object.  Not all Handlers have just one formatter and those 
formatters can contain formatters.


There are two clean ways errors should be reported in a formatter without 
changing the API.  Either throw the exception to the caller (Handler) or format 
the error using a best effort and return it as a string.

Going back to https://bugs.openjdk.java.net/browse/JDK-8028233, the pain point 
is that the error is lost when we create buggy Object.toString implementations. 
 A very clean solution would be to make the formatter simply format the 
exception, the pattern syntax (record.getMessage), the types of the arguments 
(avoids calling overridden toString methods) and return a concatinated string.  
This gets around the limitation of reporting only one failure and is not 
hostile which is nice for compatiblity.  Since we are dealing with 
formatMessage, changing the behavior doesn't break XMLFormatter or 
SimpleFormatter because that returned string is free form.  All the API says is 
"returns a localized and formatted message" when faced with a bad command 
(pattern syntax error) it would be legal to alter the string returned in the 
failure case.

Jason

Re: RFR: jsr166 jdk9 integration wave 2

[Jason Mehrens]

Hi Peter,

I've done this trick before to perform Throwable cloning.  You have to hunt for 
the constructors in this order:
1. Walk the type of the cause and super types by calling getConstructor(String, 
type of cause). (java.io.WriteAbortedException and 
javax.mail.MessagingException)
2. Walk the type of the cause and super types by calling getConstructor(type of 
cause, String)  (I've seen this but, I can't seem to find an example)
3. getConstructor(String) + initCause (java.io.InvalidObjectException)
4. Walk the type of the cause and super types by calling getConstructor(type of 
cause). (java.awt.print.PrinterIOException)
5. getConstructor() + initCause.  (java.nio.BufferOverflowException)
6. Look at the return type of methods declared in the cause type and assume 
there is a constructor matching the type or no repeating types. 
(java.nio.charset.MalformedInputException, 
java.lang.EnumConstantNotPresentException, 
java.util.regex.PatternSyntaxException)

In the end all of that can still fail.  Exceptions can be private subclasses of 
public classes so that gets interesting with reflection too.

Jason


===
What about the 4th option (keep current behavior, but try the
best-effort with reflection to make new exception of the same type):

...
 } catch (Throwable ex) {
 if (x == null) {
 x = ex;
 } else {
 // try to create new exception with same:
 // type, cause, suppressed exceptions and
stack-trace...
 Throwable nx;
 Class xClass = x.getClass();
 try {
 Constructor xConstr =
xClass.getConstructor(Throwable.class);
 nx = (Throwable) xConstr.newInstance(x.getCause());
 } catch (Exception e1) {
 try {
 nx = (Throwable) xClass.newInstance();
 nx.initCause(x.getCause());
 } catch (Exception e2) {
 // no luck
 nx = null;
 }
 }
 if (nx != null) {
 // inherit stack-trace of original exception
 nx.setStackTrace(x.getStackTrace());
 // inherit suppressed exceptions
 for (Throwable sx : x.getSuppressed()) {
 nx.addSuppressed(sx);
 }
 // add 'ex' as a final suppressed exception
 nx.addSuppressed(ex);
 x = nx;
 }
 }
 }
 completeThrowable(x, r);

...



Note that such code and similar code in
ForkJoinTask.getThrowableException will probably have to be modified for
jigsaw to include dynamic addition of read-edge to the module of
exception class...

Regards, Peter

Re: Deprecation of LogRecord.getMillis in JDK9

[Jason Mehrens]

My original thinking was that it not so much that 3rd party code doesn't care 
about nanoseconds it just that the code was built before the concept of nano 
time was added to LogRecords.

The use cases for LogRecord.getMillis are:
1.g - Formatters for rendering the event time.
2.g - Filters for throttling bursts of events.
3.g - Comparators for ordering events along with getting the sequence for tie 
breaking.
4.g - Consumer bridges of the java.util.logging framework.

The use cases for LogRecord.setMillis are:
1.s - Unit tests.
2.s - Logging framework bridges.
3.s - Type conversions of LogRecords to another subclass of LogRecord 
(GlassFish)

Note that even the logging framework itself doesn't call setMillis because the 
time is set in the LogRecord constructor.   All of the uses of setMillis are 
closer to the producer of a log record and the uses of getMillis are closer to 
the consumer of a log record.  Ideally if we are choose to lose precision it 
shouldn't impact other consumers down stream.

The reason to keep setMillis deprecated is that calling this method makes the 
choice to lose precision ahead of other code that was interested in that extra 
precision.  Out of the list of getMillis callers, 4.g is about the only reason 
to deprecate getMillis (assuming the bridge has support for nanos).  Everyone 
else is choosing their fate without choosing the fate of others.

Under the current proposal, I would have to suppress or add "some fine hackery" 
to unit tests which seems like a fair trade off since that keeps the code smell 
out of the released production code.

Jason




From: Stuart Marks <stuart.ma...@oracle.com>
Sent: Tuesday, December 1, 2015 7:13 PM
To: Daniel Fuchs
Cc: Jason Mehrens; Core-Libs-Dev
Subject: Re: Deprecation of LogRecord.getMillis in JDK9

I think #3 or a variation is best. Clearly, #1 is inconsistent and simply
documenting it (#2) isn't much better.

I'd recommend making setInstant() be more explicit about the range of Instant
values that are allowed, namely those created from Instant.ofEpochMilli(long),
which allows +/- 292 million years from the epoch. Otherwise the reader is
forced to try to understand when Instant.toEpochMilli() throws the exception.

Now that we've constrained the range of instants that a LogRecord can contain,
is it still necessary for setMillis(long) to be deprecated? Every value is
valid. It can set (almost) the full range of Instant values, just not with
nanosecond resolution. If you don't care about nanosecond resolution, this seems
like a perfectly fine method to use.

s'marks

On 12/1/15 10:48 AM, Daniel Fuchs wrote:
> Hi Jason, Stuart,
>
> Here is a potential fix for the issue:
>
> http://cr.openjdk.java.net/~dfuchs/webrev_8144262/webrev.00/src/java.logging/share/classes/java/util/logging/LogRecord.java.frames.html
>
>
> http://cr.openjdk.java.net/~dfuchs/webrev_8144262/specdiff-logging/java/util/logging/LogRecord.html
>
>
>
> As Stuart noted, java.time.Instant has a greater range than what can
> be constructed from a long milliseconds-since-epoch + a nano-time
> adjustment. This does not apply to instants returned by the system
> clock, since those are constructed precisely from such long
> milliseconds-since-epoch + nano-time adjustment.
>
> However - someone could conceivably construct such an Instant
> and pass it to a LogRecord. If that happens, then LogRecord.getMillis()
> could potentially throw an undocumented ArithmeticException.
>
> So we have at least 3 possibilities:
>
> 1. do nothing
> 2. document that getMillis() can throw ArithmeticException, with the
> additional consequence that serializing a LogRecord thus constructed
> would also throw an ArithmeticException.
> 3. modify setInstant() to validate that the instant will fit in a
> long milliseconds-since-epoch.
>
>
> The above patch implements option 3 (which currently has my
> preference). Is that the best solution?
>
> I would very much like to hear your opinion.
> If it seems like the best then I'll add a unit test, send an RFR, and
> do the paper work for the spec change...
>
> best regards, and thanks for all the valuable feedback!
>
> -- daniel
>
>
>
> On 30/11/15 18:04, Jason Mehrens wrote:
>> Hi Daniel,
>>
>>
>> When JDK-8072645 - java.util.logging should use java.time to get more precise
>> time stamps was commited the LogRecord.getMillis() method was marked as
>> deprecated with the reason "To get the full nanosecond resolution event time,
>> use getInstant".  I can see marking LogRecord.setMillis as deprecated since
>> using that would be an untended loss of precision.  However, it seems
>> excessive to deprecate LogRecord.getMillis when it could be treated as a
>> convenience metho

Re: RFR 8144262: LogRecord.getMillis() method is a convenience API that should not have been deprecated

[Jason Mehrens]

+1

From: Daniel Fuchs <daniel.fu...@oracle.com>
Sent: Wednesday, December 2, 2015 2:10 PM
To: core-libs-dev
Cc: Stuart Marks; Jason Mehrens
Subject: RFR 8144262: LogRecord.getMillis() method is a convenience API that 
should not have been deprecated

Hi,

Please find below a fix for
8144262: LogRecord.getMillis() method is a convenience API that
  should not have been deprecated
https://bugs.openjdk.java.net/browse/JDK-8144262


webrev:
http://cr.openjdk.java.net/~dfuchs/webrev_8144262/webrev.00

specdiff:
http://cr.openjdk.java.net/~dfuchs/webrev_8144262/specdiff-logging/java/util/logging/LogRecord.html


When 8072645:java.util.logging should use java.time to get more
  precise time stamps
was implemented we decided to deprecate LogRecord.getMillis()
and LogRecord.setMillis() in favor of the new LogRecord.getInstant()
and LogRecord.setInstant().

This may have been a bit hasty as LogRecord.getMillis() can in fact
be seen as a convenience method - a shortcut to
LogRecord.getInstant().toEpochMillis().
The only method we really wanted to deprecate was LogRecord.setMillis()
as that would truncate the instant to milliseconds:
in other words, LogRecord.setMillis(LogRecord.getMillis()) was no longer
idempotent.

This changes proposes to restore LogRecord.getMillis() to its previous
status, and also fixes LogRecord.setInstant to reject instant values
which would not fit in a long milliseconds-since-epoch - which would
have caused serialization to fail and LogRecord.getMillis() to throw
an undocumented ArithmeticException.

Rationale about this proposed change have also been discussed in this
thread:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/037039.html

best regards,

-- daniel

Re: Deprecation of LogRecord.getMillis in JDK9

[Jason Mehrens]

Hi Daniel,

I like proposed #3 solution too.  Usually is best to not allow "poisoning the 
well".  This will really help me out with supporting older platforms and 
keeping the code smell to a minimum.

Thanks for taking this on,

Jason


From: Daniel Fuchs <daniel.fu...@oracle.com>
Sent: Tuesday, December 1, 2015 12:48 PM
To: Jason Mehrens; Core-Libs-Dev; Stuart Marks
Subject: Re: Deprecation of LogRecord.getMillis in JDK9

Hi Jason, Stuart,

Here is a potential fix for the issue:

http://cr.openjdk.java.net/~dfuchs/webrev_8144262/webrev.00/src/java.logging/share/classes/java/util/logging/LogRecord.java.frames.html

http://cr.openjdk.java.net/~dfuchs/webrev_8144262/specdiff-logging/java/util/logging/LogRecord.html


As Stuart noted, java.time.Instant has a greater range than what can
be constructed from a long milliseconds-since-epoch + a nano-time
adjustment. This does not apply to instants returned by the system
clock, since those are constructed precisely from such long
milliseconds-since-epoch + nano-time adjustment.

However - someone could conceivably construct such an Instant
and pass it to a LogRecord. If that happens, then LogRecord.getMillis()
could potentially throw an undocumented ArithmeticException.

So we have at least 3 possibilities:

1. do nothing
2. document that getMillis() can throw ArithmeticException, with the
additional consequence that serializing a LogRecord thus constructed
would also throw an ArithmeticException.
3. modify setInstant() to validate that the instant will fit in a
long milliseconds-since-epoch.


The above patch implements option 3 (which currently has my
preference). Is that the best solution?

I would very much like to hear your opinion.
If it seems like the best then I'll add a unit test, send an RFR, and
do the paper work for the spec change...

best regards, and thanks for all the valuable feedback!

-- daniel



On 30/11/15 18:04, Jason Mehrens wrote:
> Hi Daniel,
>
>
> When JDK-8072645 - java.util.logging should use java.time to get more precise 
> time stamps was commited the LogRecord.getMillis() method was marked as 
> deprecated with the reason "To get the full nanosecond resolution event time, 
> use getInstant".  I can see marking LogRecord.setMillis as deprecated since 
> using that would be an untended loss of precision.  However, it seems 
> excessive to deprecate LogRecord.getMillis when it could be treated as a 
> convenience method that could simply note that if the caller wants nanosecond 
> resolution use getInstant.  It would be extremely helpful compatibility wise 
> to have this undeprecated for libs that have support pre-Java 9.  If it can't 
> be undeprecated what is the proper way to target support as low as JDK7 but 
> might end up executing on JDK9?
>
>
> Thanks,
>
>
> Jason
>

Deprecation of LogRecord.getMillis in JDK9

[Jason Mehrens]

Hi Daniel,


When JDK-8072645 - java.util.logging should use java.time to get more precise 
time stamps was commited the LogRecord.getMillis() method was marked as 
deprecated with the reason "To get the full nanosecond resolution event time, 
use getInstant".  I can see marking LogRecord.setMillis as deprecated since 
using that would be an untended loss of precision.  However, it seems excessive 
to deprecate LogRecord.getMillis when it could be treated as a convenience 
method that could simply note that if the caller wants nanosecond resolution 
use getInstant.  It would be extremely helpful compatibility wise to have this 
undeprecated for libs that have support pre-Java 9.  If it can't be 
undeprecated what is the proper way to target support as low as JDK7 but might 
end up executing on JDK9?


Thanks,


Jason

Re: Deprecation of LogRecord.getMillis in JDK9

[Jason Mehrens]

Sure.  I'm just trying to pick a path forward so that in X years when someone 
does code review (like my future self) can avoid, "What was this guy thinking 
calling such a SUPERSEDED, DANGEROUS, and CONDEMNED method?" when confronted 
with using getMillis.  I would assume JEP 277 would mark getMillis as 
SUPERSEDED only but, it still seems questionable that I would suppress the 
warning.  Which leads me toward removing the deprecation.

Thanks,

Jason


From: Daniel Fuchs <daniel.fu...@oracle.com>
Sent: Monday, November 30, 2015 11:32 AM
To: Jason Mehrens; Core-Libs-Dev
Subject: Re: Deprecation of LogRecord.getMillis in JDK9

Hi Jason,

To complete my earlier reply:

On 30/11/15 18:25, Daniel Fuchs wrote:
> If it can't be undeprecated what is the proper way to target support as
> low as JDK7 but might end up executing on JDK9?

Well - a lib using LogRecord.getMillis() could
use @SuppressWarnings("deprecation') when calling
LogRecord.getMillis() - but was that your question?

best regards,

-- daniel

Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions

[Jason Mehrens]

Alexander,

Given those constraints, what about simply modify the output string?  As in:
=
.
} catch (Exception ex) {
// Formatting failed: use localized format string.
return format + " " + toString(record, ex);
}

private String toString(LogRecord record, Exception ex) {
StringWriter sw = new StringWriter();
PrintWriter pw = new PrintWriter(sw);
Object[] params = record.getParameters();
if (params != null) {
   String sep = "";
   for (Object o : params)  {
   pw.append(sep);
   if (o != null) {
   pw.append(o.getClass().getName());
   } else {
   pw.append("null");
   }
   pw.println();
   sep = ",";
   }
}
ex.printStackTrace(pw);
return sw.toString();
}


There is wiggle room in the API docs for modifying the output string which 
would avoid a CCC.


Jason


From: Alexander Fomin <alexander.fo...@oracle.com>
Sent: Monday, November 23, 2015 9:27 AM
To: Jason Mehrens; core-libs-dev@openjdk.java.net; Daniel Fuchs; 
mandy.ch...@oracle.com
Subject: Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() 
swallows Exceptions

Hi Jason

On 20.11.2015 22:15, Jason Mehrens wrote:
> Alexander,
>
> I see your point.  It is also out of spec for Handler.setFormatter to 
> actually call Formatter.setErrorManager.  What if there are subclasses of 
> formatter that exist today with a setErrorManager method?  Also not all 
> handlers have one formatter or actually call super.setFormatter since you 
> can't unseal the super handler (GAE).

Good point. You are right, modifying Handler.setErrorManager and
Handler.setFormatter methods by the way to call
Formatter.setErrorManger() we may introduce  undesirable behavior. For
example, if  Handler is configured with a custom Formatter with a custom
ErrorManger specified, we may spoil the ErrorManager in that Formatter.

Probably the better solution would be to specify the default
ErrorManager for a Formatter in the constructor of specific Handler. But
seems we can't just call getErrorManager() in ConsoleHandler
constructor, for example, as far as the subclass isn't fully initialized
yet.

>
> I still lean toward letting the exceptions fly since there is already an 
> expectation that formatters fail.  Is the concern that the handler will fail 
> or that the program will fail?
Well, actually my initial thought was to let the exceptions to be thrown
from Formatter to corresponding logging Handler where it should be
processed and reported to a registered ErrorManager. But we've given up
from this idea because:
- would require a CCC any way.
- would probably require a release note since it will alter the existing
behavior.
- the current implementation of ErrorManager will cause a
printStackTrace() - but only the first time such an error happens in
Handler. So that, the errors in Formatter will be swallowed any way if
some error happened before.
- this approach may cause an NPE if the concrete Handler subclass has
not installed any ErrorManager and does not override reportError. So,
there's the issue of potentially breaking existing code.

So, I was looking for a solution that shouldn't change any existing
behavior, shouldn't break a code, should work in all cases, should allow
to a Formatter report about its internal errors itself without relay on
the corresponding Handler where the Exception could be intentionally
ignored.

>
> I've recently been tinkering with a custom filter that internally uses a 
> formatter and have prototyped adding get/setErrorManager method to it and it 
> just becomes a mess because to have start thinking about more corner cases 
> and adding more to the spec.
What is wrong with get/serErrorManager for Formatter? It should be just
a way for Formatter to report its internal errors.

> I really think that if you are going to add the 3 ErrorManager methods you 
> should only add them to the LogManager (JDK-6865510) as a way to unify 
> failure reporting in the LogManager itself and to deal with failures inside 
> of error manger.  As in to remove that ugly System.err code in 
> Handler.reportError.  The LogManager would work kind of like 
> Thread.defaultUncaughtExceptionHandler and the Handler.errorManager is kind 
> of like the Thread.uncaughtException handler.
Well. I'm with you on this. I would prefer the fix is a part of more
wider enhancement that unified Logger's inner errors reporting inclusive
redesign for ErrorManager and so on. Probably it's not a big deal, but
it should be some JEP for JDK9. I'm afraid it's too late for 9.

> Then to fix this bug we could report it to via 
> LogManager.getLogManager().reportError.
Well, it's

Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions

[Jason Mehrens]

Hi Daniel,

Well I'm sure the authors of the unit tests wrote code that never leaks the 
handlers they have created right? :)

If urgency or frequency of the reporting is required then capture the handler 
in getHead as formatter state.  The write code to report the exception under 
all possible states:
1. if exception present and getHead is called then report it to non null 
Handler and clear (JDK-6351685).
2. if exception happens during format and we have a handler captured from 
getHead then report otherwise store it.
3. if exception is present during getTail then report it to a non null Handler 
and clear the exception and hander.

That means you'll have to add super.getHead and super.getTail calls in 
XMLFormatter too.
 
I'm really leaning towards removing this try/catch 
(https://blogs.oracle.com/darcy/entry/kinds_of_compatibility).  Handlers 
already expect that Formatter.format->formattMessage will fail.  After all if 
they didn't fail we wouldn't have specified ErrorManager.FORMAT_FAILURE.  Take 
a look at StreamHandler.publish.  Or the handler implementation goes in the 
opposite direction and makes the publish method exception hostile which is 
already a violation of the spec.

It pains me to say it but, as long as you don't break the SLF4J bridge handler 
then you have covered most of the JUL users.

Jason



From: Daniel Fuchs <daniel.fu...@oracle.com>
Sent: Friday, November 20, 2015 9:32 AM
To: Jason Mehrens; Alexander Fomin; core-libs-dev@openjdk.java.net; 
mandy.ch...@oracle.com
Subject: Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() 
swallows Exceptions

On 20/11/15 15:47, Jason Mehrens wrote:
> Alexander,
>
> Why not just cache the last exception in the formatter and use getTail to 
> clear it and report it?  Since formatter is in the same package as Handler 
> you will have elevated access to the error manager through 
> Handler.reportError.  That also makes it so you don't have to change the 
> public API of Formatter.

Hi Jason,

That would mean that you won't see the exception until
the handler is closed. Not sure whether that matters much.
ErrorManager looks already bizarre to me. But at least
with ErrorManager it looks as if someone who cares could
set his/her own ErrorManager on the formatter (with hopefully
a more sensible implementation).

I have no specific opinion on the subject I'm in favor
of taking the solution that is the least likely to cause
compatibility issues :-)

best regards,

-- daniel

>
> Jason
>
> 
> From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on behalf of 
> Alexander Fomin <alexander.fo...@oracle.com>
> Sent: Friday, November 20, 2015 7:48 AM
> To: core-libs-dev@openjdk.java.net; Daniel Fuchs; mandy.ch...@oracle.com
> Subject: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() 
>   swallows Exceptions
>
> Hi,
> please, review this patch to report errors  in
> java.util.logging.Formatter#formatMessage().
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8137005
> Webrev: http://cr.openjdk.java.net/~dfuchs/alexander/8137005/webrev.00
>
> Summary:
>   j.u.logging.Formatter#formatMessage() swallows exceptions that
> happening during formatting of a message. In the result the exceptions
> are lost and users don't know about reasons why the message hasn't been
> formatted as expected. We would avoid to throw any exceptions in
> Formatter#formatMessage() from compatibility stand point. To report an
> error  in consistent way we have to pass ErrorManager in Formatter. It's
> require API changes. So, I'm going to file CCC when if the fix approved.
>   The suggested fix is to add 2 new methods in j.u.logging.Formatter
> to set/get an ErrorManager, update Formatter#formatMessage() to report
> errors via the ErrorManager and update Handler to pass errorManager to
> Formatter.
>
> Testing:
>   A couple of new regression tests have been created:
>   test/java/util/logging/Test8137005.java - real case provided by
> users
>   test/java/util/logging/NullErrorManagerTest.java - additional
> check to make sure no NPE showed if ErrorManager isn't set. Beside of
> this touched new API methods.
>
>   Logging regression tests have been run:
>   jdk/test/java/util/logging
>   jdk/test/closed/java/util/logging
>   jdk/test/sun/util/logging
>   All tests passed passed.
>
> JPRT:
> http://sthjprt.uk.oracle.com/archives/2015/11/2015-11-19-143523.gtee.dev/
> failures in the job are known issues and not related to the fix.
>
> Thanks,
> Alexander
>
>
>

Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions

[Jason Mehrens]

Right.  I know attachments get trashed sometimes so hopefully this hacked up 
code in-lines correctly:

=
private Exception formatThrown;
private volatile Handler lastHandler;

private void reportError(Handler h, Exception ex) {
if (h == null) {
   h = this.lastHandler; 
}

if (h != null) {
this.lastHandler = h;
if (ex == null) {
   ex = set(null); 
}

if (ex != null) {
   h.reportError(null, ex, ErrorManager.FORMAT_FAILURE);
}
} else {
if (ex != null) {
set(ex);
}
}
}

private synchronized Exception set(Exception e) {
Exception l = this.formatThrown;
this.formatThrown = e;
return l;
}
==

Then in the formatter.getHead/getTail call reportError(h, null) and in 
formatMessage call reportError(null, ex).  This example can be improved by 
clearing the handler/exception in getTail and avoid some redundant volatile 
stores but you get the idea.

Jason


From: Daniel Fuchs <daniel.fu...@oracle.com>
Sent: Friday, November 20, 2015 11:04 AM
To: Jason Mehrens; Alexander Fomin; core-libs-dev@openjdk.java.net; 
mandy.ch...@oracle.com
Subject: Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() 
swallows Exceptions

On 20/11/15 17:55, Jason Mehrens wrote:
> Hi Daniel,
>
> Well I'm sure the authors of the unit tests wrote code that never leaks the 
> handlers they have created right? :)
>
> If urgency or frequency of the reporting is required then capture the handler 
> in getHead as formatter state.  The write code to report the exception under 
> all possible states:
> 1. if exception present and getHead is called then report it to non null 
> Handler and clear (JDK-6351685).

oh - I see. I hadn't thought of that. That's actually a very good
suggestion. So the Formatter could access the Handler's ErrorManager
and if that's not null it could call  handler.errorManager.error(...)

That's what you are suggesting - right?

best regards,

-- daniel

> 2. if exception happens during format and we have a handler captured from 
> getHead then report otherwise store it.
> 3. if exception is present during getTail then report it to a non null 
> Handler and clear the exception and hander.
>
> That means you'll have to add super.getHead and super.getTail calls in 
> XMLFormatter too.
>
> I'm really leaning towards removing this try/catch 
> (https://blogs.oracle.com/darcy/entry/kinds_of_compatibility).  Handlers 
> already expect that Formatter.format->formattMessage will fail.  After all if 
> they didn't fail we wouldn't have specified ErrorManager.FORMAT_FAILURE.  
> Take a look at StreamHandler.publish.  Or the handler implementation goes in 
> the opposite direction and makes the publish method exception hostile which 
> is already a violation of the spec.
>
> It pains me to say it but, as long as you don't break the SLF4J bridge 
> handler then you have covered most of the JUL users.
>
> Jason
>
>
> ____
> From: Daniel Fuchs <daniel.fu...@oracle.com>
> Sent: Friday, November 20, 2015 9:32 AM
> To: Jason Mehrens; Alexander Fomin; core-libs-dev@openjdk.java.net; 
> mandy.ch...@oracle.com
> Subject: Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() 
> swallows Exceptions
>
> On 20/11/15 15:47, Jason Mehrens wrote:
>> Alexander,
>>
>> Why not just cache the last exception in the formatter and use getTail to 
>> clear it and report it?  Since formatter is in the same package as Handler 
>> you will have elevated access to the error manager through 
>> Handler.reportError.  That also makes it so you don't have to change the 
>> public API of Formatter.
>
> Hi Jason,
>
> That would mean that you won't see the exception until
> the handler is closed. Not sure whether that matters much.
> ErrorManager looks already bizarre to me. But at least
> with ErrorManager it looks as if someone who cares could
> set his/her own ErrorManager on the formatter (with hopefully
> a more sensible implementation).
>
> I have no specific opinion on the subject I'm in favor
> of taking the solution that is the least likely to cause
> compatibility issues :-)
>
> best regards,
>
> -- daniel
>
>>
>> Jason
>>
>> 
>> From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on behalf of 
>> Alexander Fomin <alexander.fo...@oracle.com>
>> Sent: Friday, November 20, 2015 7:48 AM
>> To: core-l

Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions

[Jason Mehrens]

Alexander,

I see your point.  It is also out of spec for Handler.setFormatter to actually 
call Formatter.setErrorManager.  What if there are subclasses of formatter that 
exist today with a setErrorManager method?  Also not all handlers have one 
formatter or actually call super.setFormatter since you can't unseal the super 
handler (GAE).

I still lean toward letting the exceptions fly since there is already an 
expectation that formatters fail.  Is the concern that the handler will fail or 
that the program will fail?

I've recently been tinkering with a custom filter that internally uses a 
formatter and have prototyped adding get/setErrorManager method to it and it 
just becomes a mess because to have start thinking about more corner cases and 
adding more to the spec.

I really think that if you are going to add the 3 ErrorManager methods you 
should only add them to the LogManager (JDK-6865510) as a way to unify failure 
reporting in the LogManager itself and to deal with failures inside of error 
manger.  As in to remove that ugly System.err code in Handler.reportError.  The 
LogManager would work kind of like Thread.defaultUncaughtExceptionHandler and 
the Handler.errorManager is kind of like the Thread.uncaughtException handler. 

Then to fix this bug we could report it to via 
LogManager.getLogManager().reportError.

Jason



From: Alexander Fomin <alexander.fo...@oracle.com>
Sent: Friday, November 20, 2015 11:27 AM
To: Jason Mehrens; core-libs-dev@openjdk.java.net; Daniel Fuchs; 
mandy.ch...@oracle.com
Subject: Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() 
swallows Exceptions

Hi Jason

On 20.11.2015 17:47, Jason Mehrens wrote:
> Alexander,
>
> Why not just cache the last exception in the formatter and use getTail to 
> clear it and report it?

 It might be a good solution, but according to spec
Formatter#getTail() method is intended to return the tail string for a
set of formatted records. With your approach it would need to modify the
spec any way with notes about possible exception messages in the queue.
 Besides that, the tail of the records should be the latest record.
I think that according to current spec it must be just that unformatted
message which had been passed to j.u.l.Formatter#formatMessage() and
failed to be formatted. Therefore the logic in Handler would be go
through all records in order to find Exceptions and report them if
found. Looks a little bit strange.
 But what I really care about is possible compatibility issue if
getHead()/Tail() overridden in user's Formatter implementation. One of
the example is j.u.l.XMLFormatter. Even if Exceptions have been cached
in base Formatter, it would have not been reachable in Handler with
current XMLFormatter.getTail() implementation. Well, we can modify
XMLFormatter.getTail() since it's Java code, but I don't think user's
will happy with an idea to modify anything in their app code.


> Since formatter is in the same package as Handler you will have elevated 
> access to the error manager through Handler.reportError.

Well, to which Handler? Formatter don't know anything about Handler(s)
that use(s) it. So, the only way in Formatter to get the reference on
some Handler are methods where it passed getHead()/getTail(). Taking
into account these methods may be overridden in user's implementations
the solution doesn't seem reliable.
 And then, with setErrorManager()/getErrorManager() user may set his
favorite ErrorManager.

Any way, taking into account that at least some spec changes would need
in any case, it would better to extend API for flexibility and reliability.

Thanks,
Alexander

> That also makes it so you don't have to change the public API of Formatter.
>
> Jason
>
> 
> From: core-libs-dev <core-libs-dev-boun...@openjdk.java.net> on behalf of 
> Alexander Fomin <alexander.fo...@oracle.com>
> Sent: Friday, November 20, 2015 7:48 AM
> To: core-libs-dev@openjdk.java.net; Daniel Fuchs; mandy.ch...@oracle.com
> Subject: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() 
>   swallows Exceptions
>
> Hi,
> please, review this patch to report errors  in
> java.util.logging.Formatter#formatMessage().
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8137005
> Webrev: http://cr.openjdk.java.net/~dfuchs/alexander/8137005/webrev.00
>
> Summary:
>   j.u.logging.Formatter#formatMessage() swallows exceptions that
> happening during formatting of a message. In the result the exceptions
> are lost and users don't know about reasons why the message hasn't been
> formatted as expected. We would avoid to throw any exceptions in
> Formatter#formatMessage() from compatibility stand point. To report an
> error  in consistent way we have to pass ErrorManager in Formatter. It's
> requi

Re: Proposed API for JEP 259: Stack-Walking API

[Jason Mehrens]

Mandy,

Thread.dumpStack should generate the stacktrace elements then capture 
System.err into a local var and lock it while writing the output.  That would 
be compatible with what was done before.

Jason


From: core-libs-dev  on behalf of Mandy 
Chung 
Sent: Friday, October 30, 2015 2:04 PM
To: core-libs-dev
Subject: Proposed API for JEP 259: Stack-Walking API

JEP 259:  http://openjdk.java.net/jeps/259

Javadoc for the proposed StackWalker API:
   http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html

A simple way to walk the stack:

   StackWalker walker = new StackWalker(StackWalker.Option.CLASS_REFERENCE);
   walker.walk((s) ->  s.filter(f -> 
interestingClasses.contains(f.getDeclaringClass())).findFirst());

The current usage of sun.reflect.Reflection.getCallerClass(int depth) can be 
replaced with this StackWalker API.

Any feedback on the proposed API is appreciated.

Mandy

P.S. webrev of the current implementation:
   http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/

Re: RFR 8135248: Add utility methods to check indexes and ranges

[Jason Mehrens]

Hi Paul,

Would it make sense to add these methods to the IndexOutOfBoundsException class 
itself or is there a compatibility worry?  Seems better to use the room in 
IndexOutOfBoundsException class file and keep these methods out of the Arrays 
class.  It is also odd that in the future the LinkedList would depend on the 
Arrays class to check bounds.

Jason


From: core-libs-dev  on behalf of Paul 
Sandoz 
Sent: Monday, September 21, 2015 8:42 AM
To: core-libs-dev
Subject: RFR 8135248: Add utility methods to check indexes and ranges

Hi,

Please review the following which adds methods to Arrays to check indexes and 
ranges:

  https://bugs.openjdk.java.net/browse/JDK-8135248
  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8135248-array-check-index-range/webrev/

The original motivation was an intrinsic method, Arrays.checkIndex, to check if 
an index is within bounds. Such an intrinsic guides HotSpot towards better 
optimisations for bounds checks using one unsigned comparison instead of two 
signed comparisons, and better eliding of integer to long conversions when an 
index is used to create an offset for Unsafe access. The end result is more 
efficient array access especially so from within unrolled loops. The VarHandles 
work will use Arrays.checkIndex for array access.

A follow up issue [1] will track the intrinsification of Arrays.checkIndex.

We thought it would be opportunistic to support two further common use-cases 
for sub-range checks, Arrays.checkFromToIndex and Arrays.
checkFromIndexSize. There is no current plan to intrinsify these methods.

Bounds checking is not difficult but it can be easy to make trivial mistakes. 
Thus it is advantageous to consolidate such checks not just from an 
optimization perspective but from a correctness and security/integrity 
perspective.

There are many areas in the JDK where such checks are performed. A follow up 
issue [2] will track updates to use the new methods.

The main challenge for these new methods is to design in such a way that

1) existing use-cases can still report the same set of exceptions with the same 
messages;
2) method byte code size is not unduly increased, thus perturbing inlining; and
3) there is a reasonable path for any future support of long indexes.

Paul.

[1] https://bugs.openjdk.java.net/browse/JDK-8042997
[2] https://bugs.openjdk.java.net/browse/JDK-8135250

RE: [9] RFR of 8042377: BufferedWriter and FilteredOutputStream.close throw IAE if flush and close throw equal exceptions

[Jason Mehrens]

If the other I/O classes are using it then it should be safe here then.  Just 
wanted make sure it wasn't creating a new issue.


Thanks,


Jason


 From: brian.burkhal...@oracle.com
 Subject: Re: [9] RFR of 8042377: BufferedWriter and 
 FilteredOutputStream.close throw IAE if flush and close throw equal exceptions
 Date: Wed, 24 Jun 2015 13:28:17 -0700
 To: core-libs-dev@openjdk.java.net

 I should however note that equivalent usage is already present in the close() 
 methods of FileInputStream, FileOutputStream, and RandomAccessFile which are 
 also in java.io. Perhaps the problem you allude to would be more pertinent to 
 java.lang classes?

 Brian

 On Jun 24, 2015, at 1:21 PM, Brian Burkhalter brian.burkhal...@oracle.com 
 wrote:

 Jason,

 Thanks for pointing this out. I am not sure of the answer. Let’s see what 
 the experts have to say.

 Thanks,

 Brian

 On Jun 24, 2015, at 1:03 PM, Jason Mehrens jason_mehr...@hotmail.com wrote:

 Not sure on this but, isn't it a little risky to import AtomicBoolean into 
 such low level class? I vaguely remember there was an issue with using 
 AtomicXXX in java.lang.Thread. Not sure if this case suffers the same fate.
 

RE: [9] RFR of 8042377: BufferedWriter and FilteredOutputStream.close throw IAE if flush and close throw equal exceptions

[Jason Mehrens]

Brian,


Not sure on this but, isn't it a little risky to import AtomicBoolean into such 
low level class?  I vaguely remember there was an issue with using AtomicXXX in 
java.lang.Thread.  Not sure if this case suffers the same fate.


Jason


 From: brian.burkhal...@oracle.com
 Subject: [9] RFR of 8042377: BufferedWriter and FilteredOutputStream.close 
 throw IAE if flush and close throw equal exceptions
 Date: Wed, 24 Jun 2015 12:04:58 -0700
 To: core-libs-dev@openjdk.java.net

 Please review at your convenience.

 Issue: https://bugs.openjdk.java.net/browse/JDK-8042377
 Patch: http://cr.openjdk.java.net/~bpb/8042377/webrev.00/

 The use of try-with-resources in FilteredOutputStream.close() is replaced 
 with explicit handling of IOExceptions potentially thrown by flush() or 
 close(). A test covering all cases is added.

 Thanks,

 Brian   

RE: RFR: 7113878: LogManager - namedLoggers should be ConcurrentHashMap instead of Hashtable

[Jason Mehrens]

For the test shouldn't you include a test for getLoggerNames to check that if 
the returned Enumeration is an instanceof Iterator then its remove method must 
throw an UnsupportedOperationException?  That makes it clear as to why you are 
using Collections.enumeration.


Jason


 Date: Mon, 30 Mar 2015 11:27:21 +0200
 From: daniel.fu...@oracle.com
 To: marti...@google.com; claes.redes...@oracle.com
 Subject: Re: RFR: 7113878: LogManager - namedLoggers should be 
 ConcurrentHashMap instead of Hashtable
 CC: core-libs-dev@openjdk.java.net

 Thanks all for your reviews!

 If I don't hear more then
 http://cr.openjdk.java.net/~dfuchs/webrev_7113878/webrev.01/

 is what I'm going to push.

 best regards,

 -- daniel

 On 28/03/15 00:39, Martin Buchholz wrote:
 Claes,

 Right you are - I stand corrected!

 On Fri, Mar 27, 2015 at 4:02 PM, Claes Redestad claes.redes...@oracle.com
 wrote:


 On 2015-03-27 21:12, Martin Buchholz wrote:

 Random advice - the default concurrency level of ConcurrentHashMap is 16,
 and that is almost always more than needed, probably also including here.


 I believe JSR-166e (JDK8 and onwards) mostly removed the concept of
 concurrency level from ConcurrentHashMap (while it's still there in some
 constructors to confuse things). I recall some tests showing that the
 difference in footprint between CHM and regular HashMap/Hashtable is now
 negligible even for empty, default-initialized instances.

 /Claes

 

RE: RFR: 8075810: LogManager.readConfiguration may throw undocumented IllegalArgumentException

[Jason Mehrens]

Looks good to me.  Thanks for taking this on!


Jason


 Date: Fri, 27 Mar 2015 17:18:45 +0100
 From: daniel.fu...@oracle.com
 To: jason_mehr...@hotmail.com; lance.ander...@oracle.com
 CC: core-libs-dev@openjdk.java.net; roger.ri...@oracle.com
 Subject: Re: RFR: 8075810: LogManager.readConfiguration may throw 
 undocumented IllegalArgumentException

 Hi,

 I received private feedback that using plain IOException
 might be preferable to using CharConversionException, as
 that doesn't exactly reflect what is going on here.

 So here is my new webrev - which includes the minor formatting
 issues found by Jason, but reverts to using plain IOException:

 http://cr.openjdk.java.net/~dfuchs/webrev_8075810/webrev.02

 If I don't hear negative feedback that's what I'm going to
 present to CCC.

 best regards,

 -- daniel

 On 25/03/15 12:41, Daniel Fuchs wrote:
 Thanks for the review Jason!

 On 24/03/15 18:01, Jason Mehrens wrote:
 Hi Daniel,

 Looks good. The only other alternative would be to use
 java.io.CharConversionException over IOException. We could even
 consider dropping the cause because the subclass of I/O exception
 would convey the same meaning.

 Here is a an updated webrev where I use
 java.io.CharConversionException instead of plain IOException.

 http://cr.openjdk.java.net/~dfuchs/webrev_8075810/webrev.01/

 Note: I had a look in the JDK sources and CharConversionException
 doesn't appear to be widely used. Is this the better alternative?
 I'd be happy with both (webrev.01 or webrev.00).
 Using CharConversionException means that we have to trust that
 Properties.load will obey its spec and only throw IAE in case
 of character conversion issues.

 Minor formatting issues with a missing space after the catch keyword

 Done. Thanks for catching it.

 and missing a tab ahead of 'props.load'

 That is an artifact of how webrev calls diff I think.
 If you look at the frames version you will see that the tab is here.

 best regards,

 -- daniel


 Jason


 
 Date: Tue, 24 Mar 2015 16:06:47 +0100
 From: daniel.fu...@oracle.com
 To: lance.ander...@oracle.com
 Subject: Re: RFR: 8075810: LogManager.readConfiguration may throw
 undocumented IllegalArgumentException
 CC: core-libs-dev@openjdk.java.net

 Hi Lance,

 Thanks for the review!

 On 24/03/15 16:01, Lance Andersen wrote:
 Hi Daniel,

 This looks OK but I might suggest clarifying that the cause could be
 set
 in the javadoc. Now that being said, I am not sure how consistent we
 are across the JDK or just make the assumption people will check the
 cause if they desire.

 Hmmm. I have the feeling that the best place for that would be i the
 release notes - rather than in the API doc (which reminds me I should
 plan to add some release note text to the issue).

 As far as backporting, unless it really needed, I would probably would
 not as a change such as this typically should be done as part of an
 errata/MR (due to change of behavior and it is not that big of an
 issue).

 Right, my thinking too. Thanks for sharing your opinion!

 best regards,

 -- daniel


 Best
 Lance
 On Mar 24, 2015, at 10:42 AM, Daniel Fuchs daniel.fu...@oracle.com
 mailto:daniel.fu...@oracle.com wrote:

 Hi,

 Please find below a fix for

 8075810: LogManager.readConfiguration may throw
 undocumented IllegalArgumentException

 https://bugs.openjdk.java.net/browse/JDK-8075810

 The proposed fix is to catch the IllegalArgumentException and
 wrap it in an IOException, since LogManager.readConfiguration
 is specified to throw IOException if there are problems reading
 from the stream.

 http://cr.openjdk.java.net/~dfuchs/webrev_8075810/webrev.00/

 Initial discussion around the issue can be seen here:
 http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-March/032348.html


 Question for reviewers: I will log a CCC for this - but I am wondering
 whether I should plan to backport the fix to earlier release?


 best regards,

 -- daniel

 http://oracle.com/us/design/oracle-email-sig-198324.gif
 http://oracle.com/us/design/oracle-email-sig-198324.gifhttp://oracle.com/us/design/oracle-email-sig-198324.gif

 http://oracle.com/us/design/oracle-email-sig-198324.gifLance
 Andersen|
 Principal Member of Technical Staff | +1.781.442.2037
 Oracle Java Engineering
 1 Network Drive
 Burlington, MA 01803
 lance.ander...@oracle.com mailto:lance.ander...@oracle.com





 

RE: RFR: 7113878: LogManager - namedLoggers should be ConcurrentHashMap instead of Hashtable

[Jason Mehrens]

The snapshot enumeration is a welcomed change.  ConcurrentHashMap has legacy 
Hashtable methods so you can save a little bit by calling namedLoggers.keys() 
instead of wrapping the key set.


Jason


 Date: Thu, 26 Mar 2015 14:32:23 +0100
 From: daniel.fu...@oracle.com
 To: david.hol...@oracle.com; core-libs-dev@openjdk.java.net
 Subject: Re: RFR: 7113878: LogManager - namedLoggers should be 
 ConcurrentHashMap instead of Hashtable

 On 26/03/15 13:28, David Holmes wrote:
 Hi Daniel,

 On 26/03/2015 10:08 PM, Daniel Fuchs wrote:
 Please find below a trivial fix for


 7113878: LogManager - namedLoggers should be ConcurrentHashMap
 instead of Hashtable

 As you say in the bug report, now that the map is always accessed within
 synchronized code this serves no purpose. The map not only doesn't need
 to be concurrent, it doesn't even need to be thread-safe! So why not
 replace with a simple HashMap?

 You found me out ;-) I should have mentioned it.
 The enumeration of keys returned from the map will be iterated
 outside of any synchronized block.
 ConcurrentHashMap makes this safe.

 best regards,

 -- daniel



 David

 https://bugs.openjdk.java.net/browse/JDK-7113878

 http://cr.openjdk.java.net/~dfuchs/webrev_7113878/webrev.00

 best regards,

 -- daniel
 

RE: RFR: 8075810: LogManager.readConfiguration may throw undocumented IllegalArgumentException

[Jason Mehrens]

Hi Daniel,
 
Looks good.  The only other alternative would be to use 
java.io.CharConversionException over IOException.  We could even consider 
dropping the cause because the subclass of I/O exception would convey the same 
meaning.


Minor formatting issues with a missing space after the catch keyword and 
missing a tab ahead of 'props.load'
 
Jason
 


 Date: Tue, 24 Mar 2015 16:06:47 +0100
 From: daniel.fu...@oracle.com
 To: lance.ander...@oracle.com
 Subject: Re: RFR: 8075810: LogManager.readConfiguration may throw 
 undocumented IllegalArgumentException
 CC: core-libs-dev@openjdk.java.net

 Hi Lance,

 Thanks for the review!

 On 24/03/15 16:01, Lance Andersen wrote:
 Hi Daniel,

 This looks OK but I might suggest clarifying that the cause could be set
 in the javadoc. Now that being said, I am not sure how consistent we
 are across the JDK or just make the assumption people will check the
 cause if they desire.

 Hmmm. I have the feeling that the best place for that would be i the
 release notes - rather than in the API doc (which reminds me I should
 plan to add some release note text to the issue).

 As far as backporting, unless it really needed, I would probably would
 not as a change such as this typically should be done as part of an
 errata/MR (due to change of behavior and it is not that big of an issue).

 Right, my thinking too. Thanks for sharing your opinion!

 best regards,

 -- daniel


 Best
 Lance
 On Mar 24, 2015, at 10:42 AM, Daniel Fuchs daniel.fu...@oracle.com
 mailto:daniel.fu...@oracle.com wrote:

 Hi,

 Please find below a fix for

 8075810: LogManager.readConfiguration may throw
 undocumented IllegalArgumentException

 https://bugs.openjdk.java.net/browse/JDK-8075810

 The proposed fix is to catch the IllegalArgumentException and
 wrap it in an IOException, since LogManager.readConfiguration
 is specified to throw IOException if there are problems reading
 from the stream.

 http://cr.openjdk.java.net/~dfuchs/webrev_8075810/webrev.00/

 Initial discussion around the issue can be seen here:
 http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-March/032348.html

 Question for reviewers: I will log a CCC for this - but I am wondering
 whether I should plan to backport the fix to earlier release?


 best regards,

 -- daniel

 http://oracle.com/us/design/oracle-email-sig-198324.gif
 http://oracle.com/us/design/oracle-email-sig-198324.gifhttp://oracle.com/us/design/oracle-email-sig-198324.gif
 http://oracle.com/us/design/oracle-email-sig-198324.gifLance Andersen|
 Principal Member of Technical Staff | +1.781.442.2037
 Oracle Java Engineering
 1 Network Drive
 Burlington, MA 01803
 lance.ander...@oracle.com mailto:lance.ander...@oracle.com



 

LogManager.readConfiguration doesn't document IAE and NPE.

[Jason Mehrens]

Daniel,

It occurred to me after reading Brian's patch for 
https://bugs.openjdk.java.net/browse/JDK-8075362  that the 
LogManager.readConfiguration methods do not document NPE or IAE that can be 
triggered by Properties.load.  Do we need to file a bug just against logging or 
should larger bug be filed to check all of the JDK code that is calling 
Properies.load?


Jason 

RE: JEP 102 Process Updates revised API draft

[Jason Mehrens]

Anything but allowing UOE to escape compareTo sounds good.


Apologies if I missed this in previous threads but, shouldn't 
ProcessHandle.compareTo compare hostname, startInstant, and then pid?  Or 
assuming they are not comparable between hosts then just startInstant and pid.


Jason




 Date: Mon, 9 Mar 2015 14:43:20 +
 From: chris.hega...@oracle.com
 To: roger.ri...@oracle.com; jason_mehr...@hotmail.com
 CC: core-libs-dev@openjdk.java.net
 Subject: Re: JEP 102 Process Updates revised API draft

 On 09/03/15 14:28, Roger Riggs wrote:
 Hi,

 The problem could be isolated to compareTo by defining the ordering if
 getPid
 throws UOE and without diluting the spec for getPid returning the native
 os process identifier.

 Yes, that would work.

 Defining the default for getPid() to return -1, might not have too big
 an impact.
 It would order the incompletely implemented Process subtypes before the
 real ones
 but the order is not usually significant except to be able to have a
 predictable iteration order
 or use TreeMap. Returning Long.MAX_VALUE as the default might be another
 option.

 That would probably be ok too, and then the UOE could be removed from
 Process.getPid() too, right? Which solves that small API wart.

 -Chris.

 Roger



 On 3/9/2015 6:10 AM, Chris Hegarty wrote:
 On 06/03/15 19:34, Jason Mehrens wrote:
 Hi Chris,


 Since getPid can throw UOE that means that compareTo could now throw
 UOE.

 Ooh... I don't like this.

 Has any consideration been given to getPid returning -1, if unknown or
 the default implementation? Or would this be any better?

 -Chris




 Jason


 
 Subject: Re: JEP 102 Process Updates revised API draft
 From: chris.hega...@oracle.com
 Date: Fri, 6 Mar 2015 11:59:28 +
 To: roger.ri...@oracle.com
 CC: core-libs-dev@openjdk.java.net

 Roger,

 I’ve taken a look at these changes in the sandbox (
 JDK-8046092-branch ). Overall I welcome this addition.

 Some comments, most of which I stuffed into a webrev based on your
 branch,
 http://cr.openjdk.java.net/~chegar/process_comments/webrev/

 1) ProcessHandle.compareTo() can drop the ClassCastException
 Also, I think the comparison is the wrong way around. It should
 be compare(this, other), rather than compare(other, this), right?

 2) I know there has been a lot of discussion about the use of CF,
 but I have a few more comments:

 a) Both onExist and onProcessExit are implemented to unconditionally
 throw UOE. Is the intention to make the implementation of these
 methods optional? If so, then UOE should be documented, If not,
 then I think they can be abstract, right?

 b) The wording in the spec talks about async functions and actions.
 I think this may be not quite right. The intention is to support, as is
 provided by CF, the ability to chain both sync and async tasks.
 [ I suggested some wording in the webrev ]

 c) Why the need for ISE if the process is the current process, and not
 just return a CF that never completes? Do you consider this an
 error situation that you want to notify, or consistency with other
 parts of the API ?

 d) I wonder if onProcessExit should have a small API note, saying
 that it is preferred over onExit, when you have a Process. Or
 something to promote its use over onExit, or briefly explain its
 existence. ( I know why it is there, but it may appear as duplication )

 e) Maybe onProcessExit would benefit from an apiNote to indicate
 that it is essentially an alternative to waitFor() ?

 3) Should ProcessHandle.getPid declare that it can throw IOE?
 Process.getPid declares UOE.

 4) ProcessHandle.Info.user() ? owner() seems more appropriate, no?

 5) The description of info() talks about fields, when it is an
 interface.
 I added some suggested rewording. Also, all methods now return
 references, so -1 can be removed. Similar for the Info class
 description.

 6) There are a couple of @since 1.9 tags missing from Process
 supportsDestroyForcibly and onProcessExit

 That’s all for now.

 -Chris.

RE: JEP 102 Process Updates revised API draft

[Jason Mehrens]

Hi Chris,

 

Since getPid can throw UOE that means that compareTo could now throw UOE.

 

Jason



 Subject: Re: JEP 102 Process Updates revised API draft
 From: chris.hega...@oracle.com
 Date: Fri, 6 Mar 2015 11:59:28 +
 To: roger.ri...@oracle.com
 CC: core-libs-dev@openjdk.java.net

 Roger,

 I’ve taken a look at these changes in the sandbox ( JDK-8046092-branch ). 
 Overall I welcome this addition.

 Some comments, most of which I stuffed into a webrev based on your branch,
 http://cr.openjdk.java.net/~chegar/process_comments/webrev/

 1) ProcessHandle.compareTo() can drop the ClassCastException
 Also, I think the comparison is the wrong way around. It should
 be compare(this, other), rather than compare(other, this), right?

 2) I know there has been a lot of discussion about the use of CF,
 but I have a few more comments:

 a) Both onExist and onProcessExit are implemented to unconditionally
 throw UOE. Is the intention to make the implementation of these
 methods optional? If so, then UOE should be documented, If not,
 then I think they can be abstract, right?

 b) The wording in the spec talks about async functions and actions.
 I think this may be not quite right. The intention is to support, as is
 provided by CF, the ability to chain both sync and async tasks.
 [ I suggested some wording in the webrev ]

 c) Why the need for ISE if the process is the current process, and not
 just return a CF that never completes? Do you consider this an
 error situation that you want to notify, or consistency with other
 parts of the API ?

 d) I wonder if onProcessExit should have a small API note, saying
 that it is preferred over onExit, when you have a Process. Or
 something to promote its use over onExit, or briefly explain its
 existence. ( I know why it is there, but it may appear as duplication )

 e) Maybe onProcessExit would benefit from an apiNote to indicate
 that it is essentially an alternative to waitFor() ?

 3) Should ProcessHandle.getPid declare that it can throw IOE?
 Process.getPid declares UOE.

 4) ProcessHandle.Info.user() ? owner() seems more appropriate, no?

 5) The description of info() talks about fields, when it is an interface.
 I added some suggested rewording. Also, all methods now return
 references, so -1 can be removed. Similar for the Info class description.

 6) There are a couple of @since 1.9 tags missing from Process
 supportsDestroyForcibly and onProcessExit

 That’s all for now.

 -Chris.


 

RE: RFR 9 8055330: (process spec) ProcessBuilder.start and Runtime.exec should throw UnsupportedOperationException ...

[Jason Mehrens]

Hi Roger,

For what it is worth, the most common production use case for me is:

1. Generate output from some source to a temp file.
2. Use ProcessBuilder to launch process (native or JVM) for using temp file.
3. If that fails with IOException fallback to opening a save dialog.
4. The end user will use #2 or #3 to save the file to a known location.
5. The end user will finish final product or get it to a machine that works 
then finish the final product.
6. (non)Profit.


So in this case you wait around for #1 to get done and as long as #2 or #3 work 
then #1 wasn't a wasted effort. In the UOE case 3 through 6 are skipped which 
is a bad deal for the end user.  This is 15+ years still in production code 
where the steps were crafted by a natural evolution of the Murphy's Law based 
environment that it exists in.  End of life for this code is not in sight.


Save your health and don't think too hard about the example, try to apply logic 
to it, or tear it down.  It is not my intent to hold this example up as 'the 
best counter argument', 'sky is falling' or to drag this thread out.  The point 
is, no where in the example does the calling code or user care about the 
distinction between never being able to create a process or creating a process 
failed this time.  For this code, throwing UOE is a breaking feature not a bug 
fix.  The only known thing was the API contract.  Everything else was assumed 
to be broken.


I do appreciate your time, Alan's time, the time you have spent on this issue, 
and your arguments as they are logical.  We just differ on applying the logic 
to this case.  I also understand the decision has been made and that will be 
the future.  I'll just have to be prepared for it and Murphy.


Respectfully,


Jason



 Date: Fri, 20 Feb 2015 13:06:05 -0800
 Subject: Re: RFR 9 8055330: (process spec) ProcessBuilder.start and 
 Runtime.exec should throw UnsupportedOperationException ...
 From: marti...@google.com
 To: roger.ri...@oracle.com
 CC: core-libs-dev@openjdk.java.net

 I totally disagree about recoverable condition. Instead of thinking
 about recovering, think instead about whether it ever makes sense to
 catch the resulting exception. See my sample code broken by the switch to
 UOE.

 On Fri, Feb 20, 2015 at 11:58 AM, Roger Riggs roger.ri...@oracle.com
 wrote:

 Hi Martin,

 As I said earlier, launching a Process when process is not implemented
 is not a recoverable condition; there are no conditions under which it will
 succeed. If the application is probing to determine what
 is supported on a platform then it should be prepared to handle UOE
 or using a test for the specific capability it requires.

 Roger



 On 2/20/2015 1:34 PM, Martin Buchholz wrote:

 One reason I keep pouring salt on this tiny wound is that throwing
 (unchecked) UOE for system-dependent failures when normally IOE is thrown
 is a fundamental design mistake for java and its checked exception design.
 I think it violates Josh's Effective Java Item 58: Use checked exceptions
 for recoverable conditions and runtime exceptions for programming errors.
 I don't think it's worth fixing places in jdk8 where this small mistake
 was
 made, but we can at least stop the incompatible worsening of existing
 APIs.

 On Fri, Feb 20, 2015 at 3:49 AM, Alan Bateman alan.bate...@oracle.com
 wrote:

 On 19/02/2015 21:54, Jason Mehrens wrote:

 I'm assuming that compatibility is given more weight vs. correcting
 choices made in the original design.

 Yes, I think we've spent more than enough time on it. In this case
 it's

 for a major release only and the compatibility impact seems to be only
 platforms or implementations that don't support launching of processes
 today but are running applications that attempt to start processes
 anyway.
 So overall it doesn't seem to be something to be overly concerned with.

 -Alan

RE: RFR 9 8055330: (process spec) ProcessBuilder.start and Runtime.exec should throw UnsupportedOperationException ...

[Jason Mehrens]

Standing with Martin on this, I wanted to note the following from the 
ProcessBuilder docs:
The exact nature of the exception is system-dependent, but it will always be a 
subclass of IOException

The type of exception thrown is the one thing that is defined in the spec.  The 
rest may be vague or have wiggle room but not the exception type.


I do appreciate the arguments on both sides but, I'm assuming that 
compatibility is given more weight vs. correcting choices made in the original 
design.


Jason




 Date: Wed, 18 Feb 2015 21:41:40 -0800
 Subject: Re: RFR 9 8055330: (process spec) ProcessBuilder.start and 
 Runtime.exec should throw UnsupportedOperationException ...
 From: marti...@google.com
 To: alan.bate...@oracle.com
 CC: core-libs-dev@openjdk.java.net

 On Thu, Feb 12, 2015 at 1:07 AM, Alan Bateman alan.bate...@oracle.com
 wrote:

 On 12/02/2015 02:08, Martin Buchholz wrote:

 Roger et al,

 Whichever way we go doesn't matter much.
 But I continue to think that IOE is a better choice than UOE and I have
 trouble seeing the motivation for the change to use UOE.
 If you wanted to provide a way to tell if subprocess support was available
 at all, then it would be better to add a new static boolean method to
 Process (but I wouldn't support that either).
 But (Roger and Alan): feel free to outvote me.

 I think Roger's proposal is reasonable as this is a case where the API
 will consistently throw UOE when the underlying runtime or operating system
 doesn't support a means to start processes. It's consistent with what was
 done for RMI activiation in JDK 8 (this was also about starting processes).
 Another example that comes to mind is the GUI libraries where
 HeadlessException is thrown (HeadlessException is a UOE). In the file
 system API then UOE is also specified when trying to use optional features
 that aren't supported by the implementation. There are probably many
 counter examples as we don't have consistency everywhere. In practical
 terms then I don't think this change should have an impact, but could be
 useful for those trying to take an existing app and run it on something
 like iOS. If that app relies on Runtime.exec or ProcessBuilder then the UOE
 should make it easy to identify that this part of the code needs to be
 looked at. If someday it is possible to start processes on such devices
 then an updated port to that platform wouldn't throw UOE anymore.

 -Alan.


 If you threw a variant of UOE that was also an IOException, it would have
 been equally clear to users, and no change to the contract of
 ProcessBuilder would have been necessary.

 You've broken users who relied on the old spec.   
   

RE: RFR: 8072645: java.util.logging should use java.time to get more precise time stamps

[Jason Mehrens]

Daniel,


Was it determined if you were going to modify the existing logger.dtd or create 
a logger-v2.dtd?  If you are going to create a v2 then I think it might make 
sense to make dtd log manger property for the XMLFormatter instead of 
useInstant property.  So if you are using v2 then instant is enabled.  V3 would 
be instant enabled and foo enabled and so on.



Jason




 Date: Tue, 17 Feb 2015 20:33:15 +0100
 From: daniel.fu...@oracle.com
 To: scolebou...@joda.org; core-libs-dev@openjdk.java.net; 
 peter.lev...@gmail.com
 Subject: Re: RFR: 8072645: java.util.logging should use java.time to get more 
 precise time stamps

 Hi,

 Here is a new webrev - which should contain all the feedback received
 so far:

 #1 LogRecord uses serialPersistentFields for serialization, and
 contains an Instant instead of the two fields millis +
 nanoAdjustment.
 #2 getMillis/setMillis are deprecated, replaced by getInstant/setInstant
 getNanoAdjustment/setNanoAdjustment have been dropped.

 [Thanks Peter for prototyping these 2 first changes!]

 #3 XMLFormatter uses ISO_INSTANT to print the instant in the date field.
 new constructor has been dropped. printNanos property is renamed into
 useInstant.
 #4 SimpleFormatter still uses ZonedDateTime - for compatibility and
 flexibility and because it proved to be faster than Date [1].
 #5 Tests have been updated WRT to the changes above.
 #6 various doc tweaks compared to last webrev

 new webrev:
 http://cr.openjdk.java.net/~dfuchs/webrev_8072645/webrev.01/

 specdiff updated in place (unfortunately the serial form does
 not show up):
 http://cr.openjdk.java.net/~dfuchs/webrev_8072645/specdiff-logging-time/overview-summary.html

 best regards,

 -- daniel

 [1] benchmarks related to formatting the date:
 http://cr.openjdk.java.net/~dfuchs/webrev_8072645/benchmarks.html


 On 16/02/15 20:24, Daniel Fuchs wrote:
 Hi Stephen,

 Thanks again for your support and suggestions!

 On 14/02/15 14:03, Daniel Fuchs wrote:
 If I'm not mistaken the previous SimpleFormatter used to use
 java.util.Date
 and printed the time in the local time zone. I have tried to keep this
 behavior.
 I'm not sure we would want to change it to print the time in the UTC
 time zone
 by default. A lot of developers use logging for debugging - and when
 reading
 debug messages on the console I usually prefer to see the time in my own
 time zone.

 Would there be a more efficient way to keep the default formatting of
 the time
 in the SimpleFormatter?

 I spent part of the day reading the documentation  trying out the
 various TemporalAccessors and DateTimeFormatters...

 I have also done some microbenchmark measurements (jmh) WRT
 the performance of formatting a date/time.
 Results can be seen here [1]:
 http://cr.openjdk.java.net/~dfuchs/webrev_8072645/benchmarks.html

 What it shows is that when using String.format (as the SimpleFormatter
 is specified to do), then using ZonedDateTime is actually an improvement
 over using Date.

 Now that I have read a bit more about LocalDateTime, ZonedDateTime,
 Date, and Instant, I do agree with you that for recording an event time,
 printing the Instant is the better alternative.
 However, since SimpleFormatter has always printed the local date,
 I would tend to want to keep it that way.

 So I'm going to propose to keep using ZonedDateTime in
 the SimpleFormatter, as I believe it's what gives it the maximum of
 flexibility - WRT to using String.format (+ we will get some
 boost as bonus by no longer using Date).
 If you strongly feel that java.util.logging should offer a better
 alternative - then maybe we should log an RFE to explore it?

 Things are - I believe - a bit different for the XMLFormatter.
 First, the XMLFormatter is not specified to use String.format, so
 it gives us a bit more flexibility.
 In addition we already need to tweak the format in order to introduce
 the new nanos element - (or should it be nanoOfMilli as Peter
 suggested?).

 So for the XMLFormatter, and given the results of my
 microbenchmark [1], here is what I would suggest:

 #1 rename the property printNanos into useInstant
 #2 if useInstant is false, use the old code for formatting the
 date (the old appendISO8601() method) and omit the nanos
 element - so applications that specify useInstant=false should
 see the same formatting than before, without paying the
 performance cost that using ZonedDateTime would bring.
 #3 if useInstant is absent - or not false, then we use the 'new'
 format. The date element will contain the instant printed
 using DateTimeFormatter.ISO_INSTANT, and the nanos element
 will be printed after millis

 Does that sound right? If so I will include these changes in my
 next webrev, once I have digested the feedback sent by Peter :-)

 Best regards,

 -- daniel

 [1] microbenchmark:
 http://cr.openjdk.java.net/~dfuchs/webrev_8072645/benchmarks.html



 (The webrev is broken wrt zones as it stores ZoneId.systemDefault() in a

RE: RFR: 8072645: java.util.logging should use java.time to get more precise time stamps

[Jason Mehrens]

Daniel,


I agree with you that removing the new XMLFormatter constructor is the right 
thing to do in this case.


Jason


 Date: Sat, 14 Feb 2015 13:06:21 +0100 
 From: daniel.fu...@oracle.com 
 To: jason_mehr...@hotmail.com; core-libs-dev@openjdk.java.net 
 Subject: Re: RFR: 8072645: java.util.logging should use java.time to 
 get more precise time stamps 
 
 Hi Jason, 
 
 On 2/13/15 10:57 PM, Jason Mehrens wrote: 
 
 Daniel, 
 
 
 In the XMLFormatter.format you can get rid of the double call to 
 getNanoAdjustment() since you have stored the value in the local var 'nanos'. 
 
 Thanks for spotting that, will do. 
 
 
 
 
 For the new XMLFormatter constructor what do you think about using 
 Properties, FunctionString, String, or perhaps a builder pattern? 
 
 That way if XMLFormatter is modified in the future to support 
 Throwable.getCause and Throwable.getSuppressed you don't have to keep 
 creating constructors to toggle features. 
 
 I don't know... I added the new constructor as an after thought, to help 
 writing subclasses that might not want to rely on the property defined 
 in the configuration. If we're going to rely on property anyway, then 
 the correct thing to do would be to define them as configuration properties 
 and look for them in the LogManager. 
 
 Maybe I should just remove the new constructor. 
 
 What do you think? 
 
 best regards, 
 
 -- daniel 
 
 
 
 
 
 
 Jason 
 
 
 
  
 
 
 Date: Fri, 13 Feb 2015 15:56:28 +0100 
 From: daniel.fu...@oracle.commailto:daniel.fu...@oracle.com 
 To: core-libs-dev@openjdk.java.netmailto:core-libs-dev@openjdk.java.net 
 Subject: RFR: 8072645: java.util.logging should use java.time to get more 
 precise time stamps 
 
 Hi, 
 
 Please find below a patch for: 
 
 8072645: java.util.logging should use java.time to get more 
 precise time stamps 
 
 
 http://cr.openjdk.java.net/~dfuchs/webrev_8072645/webrev.00/ 
 
 specdiff: 
 http://cr.openjdk.java.net/~dfuchs/webrev_8072645/specdiff-logging-time/java/util/logging/package-summary.html
  
 
 Overview: 
 - 
 
 The patch is made of the following pieces: 
 
 - LogRecord uses java.time.Clock's systemClock to get an 
 Instant in the best available resolution. 
 
 The instant is split into a number of milliseconds (a long) 
 and a nanosecond adjustment (an int). 
 The number of milliseconds is the same than what would have 
 been obtained by calling System.currentTimeMillis(). 
 
 - LogRecord acquires a new serializable int nanoAdjustement field, 
 which can be used together with the number of milliseconds 
 to reconstruct the instant. 
 
 - SimpleFormatter is updated to pass a ZoneDateTime 
 instance to String.format, instead of a Date. 
 
 The effect of that is that the format string can now 
 be configure to print the full instant precision, if 
 needed. 
 
 - XMLformatter will add a new nanos element after the 
 millis element - if the value of the nanoAdjustment 
 field is not 0. 
 
 The date string will also contain the nano second 
 adjustment as well as the zone offset as formatted by 
 DateTimeFormatter.ISO_OFFSET_DATE_TIME 
 
 Compatibility considerations: 
 - 
 
 - The serial for of log record is backward/forward compatible. 
 I added a test to verify that. 
 
 - XMLFormatter has acquired a new configurable property 
 'FQCN.printNanos' which allows to revert to the old 
 XML format, should the new format cause issues in 
 existing applications. 
 
 - The logger.dtd will need to be updated, to support the 
 new optional nanos element. And for this matter, 
 should we update the logger.dtd or rather define a 
 logger-v2.dtd? 
 See planned modification: 
 
 http://cr.openjdk.java.net/~dfuchs/webrev_8072645/logger-dtd/logger.dtd.frames.htmlhttp://cr.openjdk.java.net/~dfuchs/webrev_8072645/logger-dtd/logger.dtd.frames.html
  
 
 best regards, 
 
 -- daniel 
 

RE: RFR: 8072645: java.util.logging should use java.time to get more precise time stamps

[Jason Mehrens]

Daniel,


In the XMLFormatter.format you can get rid of the double call to 
getNanoAdjustment() since you have stored the value in the local var 'nanos'.  



For the new XMLFormatter constructor what do you think about using Properties, 
FunctionString, String, or perhaps a builder pattern?

That way if XMLFormatter is modified in the future to support 
Throwable.getCause and Throwable.getSuppressed you don't have to keep creating 
constructors to toggle features.



Jason




 Date: Fri, 13 Feb 2015 15:56:28 +0100
 From: daniel.fu...@oracle.com
 To: core-libs-dev@openjdk.java.net
 Subject: RFR: 8072645: java.util.logging should use java.time to get more 
 precise time stamps

 Hi,

 Please find below a patch for:

 8072645: java.util.logging should use java.time to get more
 precise time stamps


 http://cr.openjdk.java.net/~dfuchs/webrev_8072645/webrev.00/

 specdiff:
 http://cr.openjdk.java.net/~dfuchs/webrev_8072645/specdiff-logging-time/java/util/logging/package-summary.html

 Overview:
 -

 The patch is made of the following pieces:

 - LogRecord uses java.time.Clock's systemClock to get an
 Instant in the best available resolution.

 The instant is split into a number of milliseconds (a long)
 and a nanosecond adjustment (an int).
 The number of milliseconds is the same than what would have
 been obtained by calling System.currentTimeMillis().

 - LogRecord acquires a new serializable int nanoAdjustement field,
 which can be used together with the number of milliseconds
 to reconstruct the instant.

 - SimpleFormatter is updated to pass a ZoneDateTime
 instance to String.format, instead of a Date.

 The effect of that is that the format string can now
 be configure to print the full instant precision, if
 needed.

 - XMLformatter will add a new nanos element after the
 millis element - if the value of the nanoAdjustment
 field is not 0.

 The date string will also contain the nano second
 adjustment as well as the zone offset as formatted by
 DateTimeFormatter.ISO_OFFSET_DATE_TIME

 Compatibility considerations:
 -

 - The serial for of log record is backward/forward compatible.
 I added a test to verify that.

 - XMLFormatter has acquired a new configurable property
 'FQCN.printNanos' which allows to revert to the old
 XML format, should the new format cause issues in
 existing applications.

 - The logger.dtd will need to be updated, to support the
 new optional nanos element. And for this matter,
 should we update the logger.dtd or rather define a
 logger-v2.dtd?
 See planned modification:

 http://cr.openjdk.java.net/~dfuchs/webrev_8072645/logger-dtd/logger.dtd.frames.html

 best regards,

 -- daniel   

RE: RFR: 8060132: Handlers configured on abstract nodes in logging.properties are not always properly closed

[Jason Mehrens]

The strong reference is not changed to weak if later on all handlers are 
removed from the logger.



The only other solution I can think of to satisfy all of the previous pain 
points is to go back to keeping a reference to Logger.handlers in 
LogManager.LogNode and create a LogManager.orphanedHandlers mapString, COW.  
On dispose, if the handlers list is not empty, place it in the orphan handler 
map.  Every time we demand a Logger, remove the orphaned handlers list for that 
logger name and directly place the reference into the new logger.handlers.   
Then the loggers can be GC'ed but the handlers can't.  On LogManager.reset walk 
the orphaned handlers map and close all handlers.  That fixes issues with same 
handler being added multiple loggers and allows weak references to the logger.


The new pain point of this is what do you do about loading the logger handlers 
from the config file if you have an orphan handler list.  Do you skip loading 
them or just add those plus the orphan list?


Jason


 Date: Fri, 24 Oct 2014 11:31:50 -0700
 From: mandy.ch...@oracle.com
 To: daniel.fu...@oracle.com; core-libs-dev@openjdk.java.net
 Subject: Re: RFR: 8060132: Handlers configured on abstract nodes in 
 logging.properties are not always properly closed


 On 10/10/2014 8:39 AM, Daniel Fuchs wrote:
 http://cr.openjdk.java.net/~dfuchs/webrev_8060132/webrev.01

 Sorry for the delay. I have been pondering if there is a better
 alternative and looks like you have considered a few other options that
 none of them is a good one.

 typos:

 174 explicitely
 925: persitentLoggers

 Is this only problem to the abstract node of Loggers (i.e. the ancestor
 is in the logging configuration but the library/app never creates such
 a logger)?

 Your patch will keep all loggers defined in the configuration strong
 reference. What if the application really wants the loggers say
 com.foo.FooLogger to get GC'ed (e.g. it wants the logger class and
 its defining class loader to be garbage collected) but the configuration
 does name com.foo.FooLogger.handler = 

 Would that cause memory leak?

 Mandy   

RE: JDK-6774110 lock file is not deleted when child logger is used

[Jason Mehrens]

Hi Daniel, Stanimir, 


Closing the Handler is the main goal which takes care of the lock files.  As 
far as a strong reference to the logger you don't need that.  What you need to 
do is store a reference to the Logger.handlers List in the 
LogManager$LoggerWeakRef and reap the handlers inside of dispose.


Now the only other issue is if one handler has been added to multiple loggers 
which could close a handler early.  That is so uncommon I don't think it is 
worth the effort to correct.  The caller can just fix the code to use a strong 
reference.


Jason



 Date: Fri, 10 Oct 2014 11:39:41 +0200
 From: daniel.fu...@oracle.com
 To: stani...@riflexo.com
 CC: jason_mehr...@hotmail.com; core-libs-dev@openjdk.java.net
 Subject: Re: JDK-6774110 lock file is not deleted when child logger is used

 Hi Stanimir, Jason,

 On 10/10/14 10:02, Stanimir Simeonoff wrote:
 Hi,

 LogManager.reset() should invoke a package private method to delete all
 lock files. However, that would require holding the FileHandler.locks
 monitor during the resetting of FileHandlers, not just the deletion
 process. Something like that, plus new PrivilegedAction().
 static void deleteAllLocks(){
 synchronized(locks){
 for (String file : locks) new File(file).delete();
 locks.clear();
 }
 }

 There's more than the deletion of the lock file unfortunately. I believe
 the handlers should be properly closed. A handler with an XMLFormatter
 for instance needs to write the tail of the file.


 Alternatively the deletion could just be part of the Cleaner
 shutdownhook with another sun.misc.Cleaner per FileHandler that deletes
 the file. (Handlers can be shared amongst loggers, so they cannot be
 closed explicitly). There is a certain risk as file.delete() can be a
 very slow operation, though (ext3 [concurrently] deleting large files
 for example).

 That's a solution I envisaged and rejected because of the constraints
 we have when running in the ReferenceHandler thread. I don't think it
 would be appropriate to close a Handler in that thread.

 I'm leaning towards suggesting that the LogManager should hold a strong
 reference on the loggers for which a Handler is explicitly
 configured in the configuration file. It would ensure that
 these loggers are still around when reset() is called.

 best regards,

 -- daniel


 Stanimir



 On Thu, Oct 9, 2014 at 4:59 PM, Daniel Fuchs daniel.fu...@oracle.com
 mailto:daniel.fu...@oracle.com wrote:

 Thanks Jason.

 I wonder if that may be another issue. Interesting. I'll see if I
 can work out a test case
 for that tomorrow. With the test case provided in the bug - tested
 on 7, 8, and 9,
 the only file that remained at the end was 'log' (which is as it
 should be - and I ran
 the test case several times with each JDK) - which lets me think
 that maybe the
 issue was different.

 Now what you describe looks indeed like a bug that should still be
 present
 in the code base. I didn't think about that scenario, thanks for
 pointing it out!
 If i can write a reproducer (which should not be too difficult), it
 will be a good
 incentive to attempt a fix :-)

 Thanks again,

 -- daniel


 On 10/9/14 9:56 PM, Jason Mehrens wrote:

 Daniel,


 The evaluation on this bug is not quite correct. What is going
 on here is the child logger is garbage collected which makes the
 FileHandler unreachable from the LogManager$Cleaner which would
 have closed the attached FileHandler. In the example, there is
 no hard reference that escapes the 'execute' method. Prior to
 fixing JDK-6274920: JDK logger holds strong reference to
 java.util.logging.Logger instances, the LogManager$Cleaner would
 have deleted the lock file on shutdown. Now that the loggers
 are GC'able, one possible fix would be change the
 FileHandler.locks static field to MapString,FileHandler where
 the key is the file name and the value is the FileHandler that
 is open. Then in the LogManager$Cleaner could close any entries
 in that map after LogManager.reset() is executed.


 Jason



 

RE: RFR: 8060132: Handlers configured on abstract nodes in logging.properties are not always properly closed

[Jason Mehrens]

Daniel,

Looks good.  As always, thanks for fixing this.

Jason


 Date: Fri, 10 Oct 2014 17:39:55 +0200
 From: daniel.fu...@oracle.com
 To: stani...@riflexo.com; core-libs-dev@openjdk.java.net
 CC: jason_mehr...@hotmail.com
 Subject: Re: RFR: 8060132: Handlers configured on abstract nodes in 
 logging.properties are not always properly closed

 On 10/10/14 17:10, Stanimir Simeonoff wrote:
 persistentLoggers should be cleared on reset(), imo.

 Yes - I was wondering about that too, it's a bit tricky to
 get it right (WRT MT-safety) :-(.

 Also using count and then for in to loop over an array looks a bit
 ugly, should be just for (int i=0; inames.length; i++){ final String
 className = names[i];..}

 I wanted to add the logger only if at least one handler was
 successfully created and added, so 'count' is not necessarily
 equals to 'i'.

 Calling the class name 'word' is weird as well. (I do understand that's
 not new but it's a good time to get it straight).

 Agreed.

 http://cr.openjdk.java.net/~dfuchs/webrev_8060132/webrev.01

 best regards, and thanks for all the valuable inputs  comment from
 you and Jason - I really appreciate it :-)

 -- daniel


 Cheers
 Stanimir


 On Fri, Oct 10, 2014 at 5:51 PM, Daniel Fuchs daniel.fu...@oracle.com
 mailto:daniel.fu...@oracle.com wrote:

 Hi,

 Please find below a possible fix for:

 8060132: Handlers configured on abstract nodes in logging.properties
 are not always properly closed
 https://bugs.openjdk.java.net/__browse/JDK-8060132
 https://bugs.openjdk.java.net/browse/JDK-8060132

 webrev:
 http://cr.openjdk.java.net/~__dfuchs/webrev_8060132/webrev.__00/
 http://cr.openjdk.java.net/~dfuchs/webrev_8060132/webrev.00/

 Other options have been discussed in this other email thread:

 Subject: JDK-6774110 lock file is not deleted when child logger is used
 http://mail.openjdk.java.net/__pipermail/core-libs-dev/2014-__October/029038.html
 http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-October/029038.html

 best regards,

 -- daniel