[PING] RFR: JDK-8150704 XALAN: ERROR: 'No more DTM IDs are available' when transforming with lots of temporary result trees

[Langer, Christoph]

Hi,

Ping - any comments or reviews for this bugfix?

Thanks
Christoph

From: Langer, Christoph
Sent: Freitag, 26. Februar 2016 16:02
To: core-libs-dev@openjdk.java.net
Subject: RFR: JDK-8150704 XALAN: ERROR: 'No more DTM IDs are available' when 
transforming with lots of temporary result trees

Hi,

I've created a fix proposal for the issue I have reported in this bug:
https://bugs.openjdk.java.net/browse/JDK-8150704

The webrev can be found here:
http://cr.openjdk.java.net/~clanger/webrevs/8150704.1/

The Xalan parser would eventually run out of DTM IDs if xsl transformations 
involve lots of temporary result trees. Those are never released although they 
could. A testcase is included for this. I've also done some cleanups in the 
Xalan code and in the tests.

Thanks in advance for looking at this :)

Best regards
Christoph

[9] RFR: 8150702: change in javadoc for parseObject for MessageFormat - JDK-8073211

[vaibhav x.choudhary]

Hello,

Please review this small fix for jdk9/dev repo :-

Bug: https://bugs.openjdk.java.net/browse/JDK-8150702
Webrev: http://cr.openjdk.java.net/~ntv/vaibhav/8150702/webrev.00/

Reason :-
MessageFormat don't throw NullPointer exception if source is null. This 
condition is explicitly handled in the code by :-


if (source == null) {
Object[] empty = {};
return empty;
}


--
Thank You,
Vaibhav Choudhary
http://blogs.oracle.com/vaibhav

RFR: Regex exponential backtracking issue

[Xueming Shen]

Hi,

webrev: http://cr.openjdk.java.net/~sherman/regexBacktracking/webrev/

Backtracking is the powerful and popular feature provided by Java regex 
engine
when the regex pattern contains optional quantifiers or alternations, in 
which the
regex engine can return to the previous saved state (backtracking) to 
continue
matching/finding other alternative when the current try fails. However 
it is also
a "well known" issue that some not-well-written regex might trigger 
"exponential
backtraking" in situation like there is no possible match can be found 
after exhaustive
search, in the worst case the regex engine will just keep going forever, 
hangup the
vm. We have various reports in the past as showed in RegexFun.java 
 
[1] for this

issue. For example following is the one reported in  JDK-6693451.

regex:  "^(\\s*foo\\s*)*$"
input:  "foo foo foo foo foo foo foo foo foo foo foo foo foo foo 
foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo foo 
foo foo foo foo foo foo foo fo",


In which the "group" (\\s*foo\\s*)*$" can alternatively match any one of 
the "foo",
"foo ", " foo" and " foo ". The regex works fine and runs normally if it 
can find a
match (replace the last "fo" in the input to "foo" for example). But in 
this case
because the input text ends with a "fo", the regex actually can never 
match the
input, it always fails at last one, nothing in the regex can match the 
"fo". Given the
nature of the "greedy quantifier", the engine will be 
backtracking/bouncing among
these 4 choices at each/every iteration/level and keep searching to the 
end of the
input exhaustively, fail, backtrack, and try again recursively. Based on 
the number

of "foo" in the input, it can run forever until the end of the world.

There are lots of suggestions on how to avoid this kind of runaway 
regex. One is to
use the possessive quantifier, if possible, to replace the greedy 
quantifier ( * -> *+
for example ) to workaround this kind of catastrophic backtracking, and 
it normally

meets the real need and makes the exponential backtracking go away.

The observation of these exponential backtracking cases indicates that 
most of these
"exponential backtracking" however are actually not necessary in most 
cases. Even we
can't solve all of these catastrophic backtracking, it might be 
possible, with a small

cost, we can actually workaround most of the "cases", in particular

(1) when the "exponential backtracking" happens at the top level group + 
greedy

quantifier, and
(2) there is no group ref in the pattern.

The "group + greedy repetition" in j.u.regex is implemented via three 
nodes, "Prolog +
Loop + body node", in wihch the body takes care of (\\s*foo\\s*), the 
Loop takes care
of the looping for "*", and the Prolog, as its name,  just a starter to 
kick of the looping

(read source for more details).

The normal matching sequence works as

Prolog.match() ->  Prolog.loop.matchInit()

-> body.match() -> loop.match()
-> body.match() -> loop.match()
-> body.match() -> loop.match()
...
-> body.match() -> loop.match()  -> loop.next.match()


The "body.match() -> loop.match -> body.match() -> loop.match() ..." is 
looping on the

runtime call stack "recursively" (the body.next is pointing to loop).

If we take the "recursive looping" part that we are interested (we only 
care about the
"count >= cmin" situation, as it is where the exponential backtracking 
happens) out of

Loop.match(), the loop.match is as simple as

boolean match(Matcher matcher, int i, CharSequence seq) {

---> Before
boolean b = body.match(matcher, i, seq);
// If match failed we must backtrack, so
// the loop count should NOT be incremented
if (b)
return true;
matcher.locals[countIndex] = count;
---> After
return next.match(matcher, i, seq);
}

It appears that each of the repetitive "body.match() + loop.match()" 
looping (though actually
recursive on the runtime stack) actually is matching  the input text 
kind of "linearly" in the
order of loop counter, if this is a "top level" group repetition (not an 
inner group inside

another repetitive group)

... foo foo foo foo foo foo foo...
|_ i = n,
 (body-loop)(body-loop)(body-loop) ->next()

given the nature of its "greediness", the engine will exhausts all the 
possible match of
"body+loop+ loop.next" from any given starting position "i". We can 
actually assume that
if we have failed to match A "body + loop + its down-stream matching" 
last time at
position "i", next time if we come back here again (after backtracking 
the "up stream"
and come down here again), and if we are at exactly the same position 
"i", we no longer
need to try it again, the result will not change (failed). With t

Re: [DING] Re: [PING] Potential infinite waiting at JMXConnection#createConnection

[KUBOTA Yuji]

Hi Roger,

Thank you for your help!
My patch and reproducer are as below.

* patch
diff --git a/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
b/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
--- a/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
+++ b/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
@@ -222,20 +222,34 @@
 // choose protocol (single op if not reusable socket)
 if (!conn.isReusable()) {
 out.writeByte(TransportConstants.SingleOpProtocol);
 } else {
 out.writeByte(TransportConstants.StreamProtocol);
+
+int usableSoTimeout = 0;
+try {
+/*
+ * If socket factory had set a non-zero timeout on its
+ * own, then restore it instead of using the property-
+ * configured value.
+ */
+usableSoTimeout = sock.getSoTimeout();
+if (usableSoTimeout == 0) {
+  usableSoTimeout = responseTimeout;
+}
+sock.setSoTimeout(usableSoTimeout);
+} catch (Exception e) {
+// if we fail to set this, ignore and proceed anyway
+}
 out.flush();

 /*
  * Set socket read timeout to configured value for JRMP
  * connection handshake; this also serves to guard against
  * non-JRMP servers that do not respond (see 4322806).
  */
-int originalSoTimeout = 0;
 try {
-originalSoTimeout = sock.getSoTimeout();
 sock.setSoTimeout(handshakeTimeout);
 } catch (Exception e) {
 // if we fail to set this, ignore and proceed anyway
 }

@@ -279,18 +293,11 @@
  * connection.  NOTE: this timeout, if configured to a
  * finite duration, places an upper bound on the time
  * that a remote method call is permitted to execute.
  */
 try {
-/*
- * If socket factory had set a non-zero timeout on its
- * own, then restore it instead of using the property-
- * configured value.
- */
-sock.setSoTimeout((originalSoTimeout != 0 ?
-   originalSoTimeout :
-   responseTimeout));
+sock.setSoTimeout(usableSoTimeout);
 } catch (Exception e) {
 // if we fail to set this, ignore and proceed anyway
 }

 out.flush();

* reproducer
** tree
|-- debugcontroltest.properties
|-- jmx-test-cert.pkcs12
|-- jmxremote.password
|-- pom.xml
`-- src
`-- main
|-- java
|   `-- debugcontrol
|   |-- DebugController.java
|   |-- client
|   |   `-- JMXSSLClient.java
|   `-- server
|   `-- JMXSSLServer.java
`-- resources
`-- jmxremote.password

** debugcontroltest.properties
debugcontroltest.host = localhost
debugcontroltest.port = 9876
debugcontroltest.stop_time = 12

debugcontroltest.jmxremote.password.filename = jmxremote.password
debugcontroltest.cert.filename = jmx-test-cert.pkcs12
debugcontroltest.cert.type = pkcs12
debugcontroltest.cert.pass = changeit

sun.rmi.transport.tcp.responseTimeout = 1000
sun.rmi.transport.tcp.handshakeTimeout = 1000

** jmx-test-cert.pkcs12
*** binary file. Create pkcs12 certificates by openssl or other commands.

** jmxremote.password (top directory)
monitorRole adminadmin
controlRole adminadmin

** jmxremote.password (resources directory)
*** empty file. just try `touch src\main\resources\jmxremote.password`

** pom.xml

http://maven.apache.org/POM/4.0.0";
 xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
 xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
http://maven.apache.org/xsd/maven-4.0.0.xsd";>
4.0.0

debugcontrol
debugcontrol
1.0-SNAPSHOT



** DebugController.java
package debugcontrol;

import java.io.BufferedReader;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.io.OutputStream;
import java.io.PrintWriter;
import java.nio.file.FileSystem;
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.attribute.AclEntry;
import 

Re: JDK 9 RFR of JDK-8151226: Mark UdpTest.java as intermittently failing

[Lance Andersen]

looks ok joe
On Mar 3, 2016, at 6:01 PM, joe darcy  wrote:

> Hello,
> 
> The test
> 
>java/net/ipv6tests/UdpTest.java
> 
> has been observed to intermittently fail (JDK-8143998, JDK-8143097).
> 
> Until these problems are addressed, the test should be marked accordingly.
> 
> Please review the patch below which marks the test.
> 
> Thanks,
> 
> -Joe
> 
> diff -r a603b1f1d9a1 test/java/net/ipv6tests/UdpTest.java
> --- a/test/java/net/ipv6tests/UdpTest.javaThu Mar 03 12:49:12 2016 -0800
> +++ b/test/java/net/ipv6tests/UdpTest.javaThu Mar 03 15:01:29 2016 -0800
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2003, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2003, 2016, 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
> @@ -24,6 +24,7 @@
> /*
>  * @test
>  * @bug 4868820
> + * @key intermittent
>  * @summary IPv6 support for Windows XP and 2003 server
>  */
> 
> 



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

JDK 9 RFR of JDK-8151226: Mark UdpTest.java as intermittently failing

[joe darcy]

Hello,

The test

java/net/ipv6tests/UdpTest.java

has been observed to intermittently fail (JDK-8143998, JDK-8143097).

Until these problems are addressed, the test should be marked accordingly.

Please review the patch below which marks the test.

Thanks,

-Joe

diff -r a603b1f1d9a1 test/java/net/ipv6tests/UdpTest.java
--- a/test/java/net/ipv6tests/UdpTest.javaThu Mar 03 12:49:12 2016 -0800
+++ b/test/java/net/ipv6tests/UdpTest.javaThu Mar 03 15:01:29 2016 -0800
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2003, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2003, 2016, 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
@@ -24,6 +24,7 @@
 /*
  * @test
  * @bug 4868820
+ * @key intermittent
  * @summary IPv6 support for Windows XP and 2003 server
  */

Re: RFR:JDK-8032051:"ZonedDateTime" class "parse" method fails with short time zone offset ("+01")

[Roger Riggs]

Hi Nadeesh,

Looks good.

Thanks, Roger


On 3/3/2016 1:37 PM, nadeesh tv wrote:

Hi,

Stephen, Roger Thanks for the comments.

Please see the updated webrev 
http://cr.openjdk.java.net/~ntv/8032051/webrev.04/



Regards,
Nadeesh


On 3/1/2016 12:29 AM, Stephen Colebourne wrote:

I'm happy to go back to the spec I proposed before. That spec would
determine colons dynamically only for pattern HH. Otherwise, it would
use the presence/absence of a colon in the pattern as the signal. That
would deal with the ISO-8601 problem and resolve the original issue
(as ISO_OFFSET_DATE_TIME uses HH:MM:ss, which would leniently parse
using colons).

Writing the spec wording is not easy however. I had:

When parsing in lenient mode, only the hours are mandatory - minutes
and seconds are optional. The colons are required if the specified
pattern contains a colon. If the specified pattern is "+HH", the
presence of colons is determined by whether the character after the
hour digits is a colon or not. If the offset cannot be parsed then an
exception is thrown unless the section of the formatter is optional.

which isn't too bad but alternatives are possible.

Stephen




On 29 February 2016 at 15:52, Roger Riggs  
wrote:

Hi Stephen,

As a fix for the original issue[1], not correctly parsing a ISO defined
offset, the use of lenient
was a convenient implementation technique (hack).  But with the 
expanded

definition of lenient,
it will allow many forms of the offset that are not allowed by the ISO
specification
and should not be accepted forDateTimeFormatter. ISO_OFFSET_DATE_TIME.
In particular, ISO requires the ":" to separate the minutes.
I'm not sure how to correctly fix the original issue with the new
specification of lenient offset
parsing without introducing some more specific implementation 
information.



WRT the lenient parsing mode for appendOffset:

I was considering that the subfields of the offset were to be treated
leniently but it seems
you were treating the entire offset field and text as the unit to be 
treated

leniently.
The spec for lenient parsing would be clearer if it were specified as
allowing any
of the patterns of appendOffset.  The current wording around the 
character

after the hour
may be confusing.

In the specification of appendOffset(pattern, noOffsetText) how about:

"When parsing in lenient mode, the longest valid pattern that 
matches the

input is used. Only the hours are mandatory, minutes  and seconds are
optional."

Roger


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





On 2/26/2016 1:10 PM, Stephen Colebourne wrote:

It is important to also consider the case where the user wants to
format using HH:MM but parse seconds if they are provided.

As I said above, this is no different to SignStyle, where the user
requests something specific on format, but accepts anything on input.

The pattern is still used for formatting and strict parsing under
these changes. It is effectively ignored in lenient parsing (which is
the very definition of leniency).

Another way to look at it:

using a pattern of HH:MM and strict:
+02 - disallowed
+02:00 - allowed
+02:00:00 - disallowed

using a pattern of HH:mm and strict:
+02 - allowed
+02:00 - allowed
+02:00:00 - disallowed

using any pattern and lenient:
+02 - allowed
+02:00 - allowed
+02:00:00 - allowed

This covers pretty much anything a user needs when parsing.

Stephen


On 26 February 2016 at 17:38, Roger Riggs  
wrote:

Hi Stephen,

Even in lenient mode the parser needs to stick to the fields 
provided in

the
pattern.
If the caller intends to parse seconds, the pattern should include
seconds.
Otherwise the caller has not been able to specify their intent.
That's consistent with lenient mode used in the other fields.
Otherwise, the pattern is irrelevant except for whether it 
contains a ":"

and makes
the spec nearly useless.

Roger



On 2/26/2016 12:09 PM, Stephen Colebourne wrote:

On 26 February 2016 at 15:00, Roger Riggs 
wrote:

Hi Stephen,

It does not seem natural to me with a pattern of HHMM for it to 
parse

more
than 4 digits.
I can see lenient modifying the behavior as it it were HHmm, but 
there

is
no
indication in the pattern
that seconds would be considered.  How it would it be implied 
from the

spec?

The spec is being expanded to define what happens. Previously it
didn't define it at all, and would throw an error.

Lenient parsing typically accepts much more than the strict parsing.

When parsing numbers, you may set the SignStyle to NEVER, but the 
sign

will still be parsed in lenient mode

When parsing text, you may select the short output format, but any
length of text will be parsed in lenient mode.

As such, it is very much in line with the behavour of the API to 
parse
a much broader format than the one requested in lenient mode. 
(None of

this affects strict mode).

Stephen


In the original issue, appendOffsetId is defined as using the 
+HH:MM:ss

pattern and
specific to ISO the MM should be allowed to be opt

Re: [DING] Re: [PING] Potential infinite waiting at JMXConnection#createConnection

[Roger Riggs]

Hi Yuji,

An issue has been created to track this issue:

JDK-8151212  Flush in 
RMI TCPChannel createConnection can hang indefinitely


Please send the patch and the reproducer in the body of email and I'll 
attach them to the bug report.


Thanks, Roger



On 2/3/2016 12:27 PM, KUBOTA Yuji wrote:

Hi all,

Could someone please review and sponsor this fix ?
I write the details of this issue again. Please review it.

=Problem=
Potential infinite waiting at TCPChannel#createConnection.

This method flushes the DataOutputStream without the socket
timeout settings when choose stream protocol [1]. If connection lost
or the destination server do not return response during the flush,
this method wait forever because the timeout settings is set the
default value of SO_TIMEOUT, i.e., infinite.

[1]: 
http://hg.openjdk.java.net/jdk9/dev/jdk/file/7adef1c3afd5/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java#l227

I think this issue is rarely, however serious.

=Reproduce=
I write a test program to reproduce. You can reproduce by the below.

* hg clone 
http://icedtea.classpath.org/people/ykubota/fixLoopAtJMXConnectorFactory/
* cd fixLoopAtJMXConnectorFactory; mvn package
* setting "stop_time" at debugcontrol.properties if you need.
* java -cp .:target/debugcontrol-1.0-SNAPSHOT.jar debugcontrol.DebugController

This program keep to wait at TCPChannel#createConnection due to
this issue. After "debugcontroltest.stop_time" ms, this program release
the waiting by sending quit to jdb which is stopping the destination
server. Finally, return 2.

=Solution=
Set timeout by using property-configured value:
sun.rmi.transport.tcp.responseTimeout.

My patch is below.
http://icedtea.classpath.org/people/ykubota/fixLoopAtJMXConnectorFactory/file/e31044f0804f/jdk9.patch

If you run the test program with modified JDK9 by my patch, the test
program will get java.net.SocketTimeoutException after the connection
timeout happen, then return 0.

Thanks,
Yuji.


2016-01-13 23:31 GMT+09:00 KUBOTA Yuji :

Hi all,

Can somebody please review and sponsor this fix ?

Thanks,
Yuji

2016-01-05 17:56 GMT+09:00 KUBOTA Yuji :

Hi Jaroslav and core-libs-dev,

Thank Jaroslav for your kindness!

For core-libs-dev members, links the information about this issue.

  * details of problem
  http://mail.openjdk.java.net/pipermail/jdk9-dev/2015-April/002152.html

  * patch
  
http://icedtea.classpath.org/people/ykubota/fixLoopAtJMXConnectorFactory/file/e31044f0804f/jdk9.patch

  * testcase for reproduce
  
http://icedtea.classpath.org/people/ykubota/fixLoopAtJMXConnectorFactory/file/e31044f0804f/testProgram
  
http://mail.openjdk.java.net/pipermail/serviceability-dev/2015-December/018415.html

Could you please review these reports?
Hope this patch helps to community.

Thanks,
Yuji

2016-01-04 23:51 GMT+09:00 Jaroslav Bachorik :

Hi Yuji,

On 4.1.2016 15:14, KUBOTA Yuji wrote:

Hi all,

Could you please review this patch?


Sorry for the long delay. Shanliang has not been present for some time and
probably this slipped the attention of the others.

However, core-libs mailing list might be more appropriate place to review
this change since you are dealing with s.r.t.t.TCPChannel
(http://icedtea.classpath.org/people/ykubota/fixLoopAtJMXConnectorFactory/file/e31044f0804f/jdk9.patch)

Regards,

-JB-

Re: RFR: jsr166 jdk9 integration wave 5

[Martin Buchholz]

Committing.

On Thu, Mar 3, 2016 at 3:48 AM, Paul Sandoz  wrote:

> java/util/concurrent/ScheduledThreadPoolExecutor/DelayOverflow.java
> —
>
> -pool.schedule(keepPoolBusy, 0, TimeUnit.SECONDS);
> +pool.schedule(keepPoolBusy, 0, DAYS);
>
> It probably does not matter that you changed the units here?

Probably wouldn't have happened if DAYS was spelled HECTOKILOSECONDS !

Re: RFR:JDK-8030864:Add an efficient getDateTimeMillis method to java.time

[nadeesh tv]

Hi,

Roger - Thanks for the comments

Made the necessary changes in the spec

Please see the updated webrev 
http://cr.openjdk.java.net/~ntv/8030864/webrev.05/

On 3/3/2016 12:21 AM, nadeesh tv wrote:

Hi ,

Please see the updated webrev 
http://cr.openjdk.java.net/~ntv/8030864/webrev.03/


Thanks and Regards,
Nadeesh

On 3/3/2016 12:01 AM, Roger Riggs wrote:

Hi Nadeesh,

Editorial comments:

Chronology.java: 716+
  "Java epoch"  -> "epoch"
  "minute, second and zoneOffset"  ->  "minute, second*,* and 
zoneOffset"  (add a comma; two places)
  "caluculated using given era, prolepticYear," -> "calculated using 
the era, year-of-era,"

  "to represent" ->  remove as unnecessary in all places

IsoChronology:
  "to represent" ->  remove as unnecessary in all places

These should be fixed to cleanup the specification.

The implementation and the tests look fine.

Thanks, Roger



On 3/2/2016 10:17 AM, nadeesh tv wrote:

Hi,
Stephen, Thanks for the comments.
Please see the updated webrev
http://cr.openjdk.java.net/~ntv/8030864/webrev.02/

Regards,
Nadeesh TV

On 3/2/2016 5:41 PM, Stephen Colebourne wrote:

Remove "Subclass can override the default implementation for a more
efficient implementation." as it adds no value.

In the default implementation of

epochSecond(Era era, int yearofEra, int month, int dayOfMonth,
int hour, int minute, int second, ZoneOffset zoneOffset)

use

prolepticYear(era, yearOfEra)

and call the other new epochSecond method. See dateYearDay(Era era,
int yearOfEra, int dayOfYear) for the design to copy. If this is done,
then there is no need to override the method in IsoChronology.

In the test,

LocalDate.MIN.with(chronoLd)

could be

LocalDate.from(chronoLd)

Thanks
Stephen






On 2 March 2016 at 10:30, nadeesh tv  wrote:

Hi all,

Please review an enhancement  for a  garbage free epochSecond method.

Bug ID: https://bugs.openjdk.java.net/browse/JDK-8030864

webrev: http://cr.openjdk.java.net/~ntv/8030864/webrev.01

--
Thanks and Regards,
Nadeesh TV









--
Thanks and Regards,
Nadeesh TV

Re: Match.appendReplacement with StringBuilder

[Xueming Shen]

On 3/3/16, 10:26 AM, Dave Brosius wrote:

Greetings,

   It would be nice if java.util.regex.Matcher had a replacement for

Matcher  appendReplacement(StringBuffer sb, String 
replacement)

StringBuffer appendTail(StringBuffer sb)


That took StringBuilder.


we have added that in 9, right?

Re: RFR:JDK-8032051:"ZonedDateTime" class "parse" method fails with short time zone offset ("+01")

[nadeesh tv]

Hi,

Stephen, Roger Thanks for the comments.

Please see the updated webrev 
http://cr.openjdk.java.net/~ntv/8032051/webrev.04/



Regards,
Nadeesh


On 3/1/2016 12:29 AM, Stephen Colebourne wrote:

I'm happy to go back to the spec I proposed before. That spec would
determine colons dynamically only for pattern HH. Otherwise, it would
use the presence/absence of a colon in the pattern as the signal. That
would deal with the ISO-8601 problem and resolve the original issue
(as ISO_OFFSET_DATE_TIME uses HH:MM:ss, which would leniently parse
using colons).

Writing the spec wording is not easy however. I had:

When parsing in lenient mode, only the hours are mandatory - minutes
and seconds are optional. The colons are required if the specified
pattern contains a colon. If the specified pattern is "+HH", the
presence of colons is determined by whether the character after the
hour digits is a colon or not. If the offset cannot be parsed then an
exception is thrown unless the section of the formatter is optional.

which isn't too bad but alternatives are possible.

Stephen




On 29 February 2016 at 15:52, Roger Riggs  wrote:

Hi Stephen,

As a fix for the original issue[1], not correctly parsing a ISO defined
offset, the use of lenient
was a convenient implementation technique (hack).  But with the expanded
definition of lenient,
it will allow many forms of the offset that are not allowed by the ISO
specification
and should not be accepted forDateTimeFormatter. ISO_OFFSET_DATE_TIME.
In particular, ISO requires the ":" to separate the minutes.
I'm not sure how to correctly fix the original issue with the new
specification of lenient offset
parsing without introducing some more specific implementation information.


WRT the lenient parsing mode for appendOffset:

I was considering that the subfields of the offset were to be treated
leniently but it seems
you were treating the entire offset field and text as the unit to be treated
leniently.
The spec for lenient parsing would be clearer if it were specified as
allowing any
of the patterns of appendOffset.  The current wording around the character
after the hour
may be confusing.

In the specification of appendOffset(pattern, noOffsetText) how about:

"When parsing in lenient mode, the longest valid pattern that matches the
input is used. Only the hours are mandatory, minutes  and seconds are
optional."

Roger


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





On 2/26/2016 1:10 PM, Stephen Colebourne wrote:

It is important to also consider the case where the user wants to
format using HH:MM but parse seconds if they are provided.

As I said above, this is no different to SignStyle, where the user
requests something specific on format, but accepts anything on input.

The pattern is still used for formatting and strict parsing under
these changes. It is effectively ignored in lenient parsing (which is
the very definition of leniency).

Another way to look at it:

using a pattern of HH:MM and strict:
+02 - disallowed
+02:00 - allowed
+02:00:00 - disallowed

using a pattern of HH:mm and strict:
+02 - allowed
+02:00 - allowed
+02:00:00 - disallowed

using any pattern and lenient:
+02 - allowed
+02:00 - allowed
+02:00:00 - allowed

This covers pretty much anything a user needs when parsing.

Stephen


On 26 February 2016 at 17:38, Roger Riggs  wrote:

Hi Stephen,

Even in lenient mode the parser needs to stick to the fields provided in
the
pattern.
If the caller intends to parse seconds, the pattern should include
seconds.
Otherwise the caller has not been able to specify their intent.
That's consistent with lenient mode used in the other fields.
Otherwise, the pattern is irrelevant except for whether it contains a ":"
and makes
the spec nearly useless.

Roger



On 2/26/2016 12:09 PM, Stephen Colebourne wrote:

On 26 February 2016 at 15:00, Roger Riggs 
wrote:

Hi Stephen,

It does not seem natural to me with a pattern of HHMM for it to parse
more
than 4 digits.
I can see lenient modifying the behavior as it it were HHmm, but there
is
no
indication in the pattern
that seconds would be considered.  How it would it be implied from the
spec?

The spec is being expanded to define what happens. Previously it
didn't define it at all, and would throw an error.

Lenient parsing typically accepts much more than the strict parsing.

When parsing numbers, you may set the SignStyle to NEVER, but the sign
will still be parsed in lenient mode

When parsing text, you may select the short output format, but any
length of text will be parsed in lenient mode.

As such, it is very much in line with the behavour of the API to parse
a much broader format than the one requested in lenient mode. (None of
this affects strict mode).

Stephen



In the original issue, appendOffsetId is defined as using the +HH:MM:ss
pattern and
specific to ISO the MM should be allowed to be optional.   There is no
question of parsing
extra digits not included in the requested pattern.

Separately, this

Match.appendReplacement with StringBuilder

[Dave Brosius]

Greetings,

   It would be nice if java.util.regex.Matcher had a replacement for

Matcher  appendReplacement(StringBuffer sb, String 
replacement)

StringBuffer appendTail(StringBuffer sb)


That took StringBuilder.

Re: RFR 8150679: closed/javax/crypto/CryptoPermission/CallerIdentification.sh fails after fix for JDK-8132734

[Steve Drach]

> On Mar 3, 2016, at 3:26 AM, Paul Sandoz  wrote:
> 
> 
>> On 2 Mar 2016, at 20:12, Steve Drach  wrote:
>> 
>> Please review the following fix for JDK-8150679
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8150679/webrev/ 
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8150679 
>> 
>> 
>> The test was modified to demonstrate the problem.
> 
> You are essentially bombing out of MR-JAR functionality if the JarEntry is 
> not an instance of JarFileEntry.

If it’s not a JarFileEntry, none of the MR functionality has been invoked.

> That might be ok for a short-term solution, but it might require some further 
> deeper investigation on things that extend JarEntry and how it is is used by 
> VerifierStream [*].
> 
> JarFile:
> 
> 895 private JarEntry verifiableEntry(ZipEntry ze) {
> 896 if (ze == null) return null;
> 
> You don’t need this. The code will anyway throw an NPE elsewhere, and the 
> original code threw an NPE when obtaining the name:

Ok.  I’ll take this out.  Feels a bit uncomfortable though.

> 
>return new JarVerifier.VerifierStream(
>getManifestFromReference(),
>ze instanceof JarFileEntry ?
>(JarEntry) ze : getJarEntry(ze.getName()),
>super.getInputStream(ze),
>jv);
> 
> 
> 897 if (ze instanceof JarFileEntry) {
> 898 // assure the name and entry match for verification
> 899 return ((JarFileEntry)ze).reifiedEntry();
> 900 }
> 901 ze = getJarEntry(ze.getName());
> 902 assert ze instanceof JarEntry;
> 
> This assertion is redundant as the method signature of getJarEntry returns 
> JarEntry.

I know it’s redundant.  It was a statement of fact. but the method signature 
does the same thing.

> 
> 
> 903 if (ze instanceof JarFileEntry) {
> 904 return ((JarFileEntry)ze).reifiedEntry();
> 905 }
> 906 return (JarEntry)ze;
> 907 }
> 
> 
> MultiReleaseJarURLConnection
> —
> 
> Given your changes above i am confused how your test passes for instances of 
> URLJarFileEntry since they cannot be reified.

I suspect that it works for regular jar files but not for MR jar files.  That’s 
another bug in URLJarFile — it gets a versioned entry that can’t be verified.  
I mentioned this yesterday.  I’ll write a test and if warranted submit a bug on 
this.

> 
> Paul.
> 
> [*] AFAICT JarVerifier directly accesses the fields JarEntry.signers/certs.

Yes, but the overriden fields are the ones of interest.  

> 

(Round 2) RFR(s): 8150460: (linux|bsd|aix)_close.c: file descriptor table may become large or may not work at all

[Thomas Stüfe]

Hi all,

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

thanks to all who took the time to review the first version of this fix!

This is the new version:

http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webrev.02/webrev/

I reworked the fix, trying to add in all the input I got: This fix uses a
simple one-dimensional array, preallocated at startup, for low-value file
descriptors. Like the code did before. Only for large values of file
descriptors it switches to an overflow table, organized as two dimensional
sparse array of fixed sized slabs, which are allocated on demand. Only the
overflow table is protected by a lock.

For 99% of all cases we will be using the plain simple fdTable structure as
before. Only for unusually large file descriptor values we will be using
this overflow table.

Memory footprint is kept low: for small values of RLIMIT_NOFILE, we will
only allocate as much space as we need. Only if file descriptor values get
large, memory is allocated in the overflow table.

Note that I avoided the proposed double-checked locking solution: I find it
too risky in this place and also unnecessary. When calling getFdEntry(), we
will be executing a blocking IO operation afterwards, flanked by two mutex
locks (in startOp and endOp). So, I do not think the third mutex lock in
getFdEntry will add much, especially since it is only used in case of
larger file descriptor values.

I also added the fix to bsd_close.c and aix_close.c. I do not like this
code triplication. I briefly played around with unifying this code, but
this is more difficult than it seems: implementations subtly differ between
the three platforms, and solaris implementation is completely different. It
may be a worthwhile cleanup, but that would be a separate issue.

I did some artificial tests to check how the code does with many and large
file descriptor values, all seemed to work well. I also ran java/net jtreg
tests on Linux and AIX.

Kind Regards, Thomas

Re: RFR: 8147755: ASM should create correct constant tag for invokestatic on handle point to interface static method

[forax]

comments inlinined ...

- Mail original -
> De: "Kumar Srinivasan" 
> À: "core-libs-dev" , "Remi Forax" 
> 
> Cc: "SUNDARARAJAN.ATHIJEGANNATHAN" , 
> "Michael Haupt"
> , "Jaroslav Bachor?k" 
> , "Coleen Phillmore"
> , "Stas Smirnov" 
> , "harsha wardhana b"
> 
> Envoyé: Mercredi 2 Mars 2016 16:41:01
> Objet: RFR: 8147755: ASM should create correct constant tag for invokestatic 
> on handle point to interface static
> method
> 
> Hello Remi, et. al.,

Hi Kumar,

> 
> Webrev:
> http://cr.openjdk.java.net/~ksrini/8147755/webrev.00/
> 
> Can you please approve this patch, it is taken out of ASM's svn repo.
> change id 1795,  which addresses the problem described in [1].

for anybody else, this revision roughtly correspond to ASM 5.1, which includes:
 - change non visible StringBuffer to StringBuilder
 - improve documentation of Printer API (and related classe)
 - provide a new Remapper API
 - add a way to create constant method handles on interface 
(invokestatic/invokespecial)
   which fix 8147755.

> 
> Note 1: A couple of @Deprecated annotations and doc comments
> have been disabled, because we have a catch-22 that an internal and closed
> component depends on these APIs, and the replacement is not available until
> we push this patch. A follow up bug [2] has been filed.

for ASM, we use @Deprecated when a method is superseded by a new one,
not to indicate that calling this method is hazardous,
so i'm fine if you want to do the update in two steps.

> 
> Note 2: jprt tested, all core-libs, langtools and nashorn regressions
> pass. HotSpot team has verified that it address their issues.
> 
> 
> Thank you
> Kumar

thumb up for me.

Rémi

> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8147755
> [2] https://bugs.openjdk.java.net/browse/JDK-8151056
> 

Re: RFR 8150679: closed/javax/crypto/CryptoPermission/CallerIdentification.sh fails after fix for JDK-8132734

[Claes Redestad]

Hi,

On 2016-03-03 12:26, Paul Sandoz wrote:

On 2 Mar 2016, at 20:12, Steve Drach  wrote:

Please review the following fix for JDK-8150679

webrev: http://cr.openjdk.java.net/~sdrach/8150679/webrev/ 



Looks OK to me.

issue: https://bugs.openjdk.java.net/browse/JDK-8150679 


The test was modified to demonstrate the problem.

You are essentially bombing out of MR-JAR functionality if the JarEntry is not 
an instance of JarFileEntry. That might be ok for a short-term solution, but it 
might require some further deeper investigation on things that extend JarEntry 
and how it is is used by VerifierStream [*].


I agree with Paul that this needs deeper investigation as a follow-up, 
but would like to stress that this fix addresses numerous things that is 
breaking in 9-b108, including many benchmarks. With a number of critical 
integrations planned in the next couple of weeks I think we need to 
fast-track a promotion with this fix before that happens so that we can 
provide reasonable testing.


Thanks!

/Claes

Re: [DONG] Re: [DING] Re: [PING] Potential infinite waiting at JMXConnection#createConnection

[KUBOTA Yuji]

Hi all,

Could someone please review this patch?

Thanks,
Yuji

2016-02-09 15:50 GMT+09:00 KUBOTA Yuji :
> Hi David,
>
> Thank you for your advice and cc-ing!
>
> I do not have any role yet, so I paste my patches as below.
>
> diff --git a/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
> b/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
> --- a/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
> +++ b/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java
> @@ -222,20 +222,34 @@
>  // choose protocol (single op if not reusable socket)
>  if (!conn.isReusable()) {
>  out.writeByte(TransportConstants.SingleOpProtocol);
>  } else {
>  out.writeByte(TransportConstants.StreamProtocol);
> +
> +int usableSoTimeout = 0;
> +try {
> +/*
> + * If socket factory had set a non-zero timeout on 
> its
> + * own, then restore it instead of using the 
> property-
> + * configured value.
> + */
> +usableSoTimeout = sock.getSoTimeout();
> +if (usableSoTimeout == 0) {
> +  usableSoTimeout = responseTimeout;
> +}
> +sock.setSoTimeout(usableSoTimeout);
> +} catch (Exception e) {
> +// if we fail to set this, ignore and proceed anyway
> +}
>  out.flush();
>
>  /*
>   * Set socket read timeout to configured value for JRMP
>   * connection handshake; this also serves to guard 
> against
>   * non-JRMP servers that do not respond (see 4322806).
>   */
> -int originalSoTimeout = 0;
>  try {
> -originalSoTimeout = sock.getSoTimeout();
>  sock.setSoTimeout(handshakeTimeout);
>  } catch (Exception e) {
>  // if we fail to set this, ignore and proceed anyway
>  }
>
> @@ -279,18 +293,11 @@
>   * connection.  NOTE: this timeout, if configured to a
>   * finite duration, places an upper bound on the time
>   * that a remote method call is permitted to execute.
>   */
>  try {
> -/*
> - * If socket factory had set a non-zero timeout on 
> its
> - * own, then restore it instead of using the 
> property-
> - * configured value.
> - */
> -sock.setSoTimeout((originalSoTimeout != 0 ?
> -   originalSoTimeout :
> -   responseTimeout));
> +sock.setSoTimeout(usableSoTimeout);
>  } catch (Exception e) {
>  // if we fail to set this, ignore and proceed anyway
>  }
>
>  out.flush();
>
> Thanks,
> Yuji
>
> 2016-02-09 13:11 GMT+09:00 David Holmes :
>> Hi Yuji,
>>
>> Not sure who would look at this so cc'ing net-dev.
>>
>> Also note that contributions can only be accepted if presented via OpenJKDK
>> infrastructure. Links to patches on http://icedtea.classpath.org are not
>> acceptable. The patch needs to be included in the email (beware stripped
>> attachments) if you can't get it hosted on cr.openjdk.java.net. Sorry.
>>
>> David
>>
>>
>> On 9/02/2016 12:10 AM, KUBOTA Yuji wrote:
>>>
>>> Hi all,
>>>
>>> Could someone review this fix?
>>>
>>> Thanks,
>>> Yuji
>>>
>>> 2016-02-04 2:27 GMT+09:00 KUBOTA Yuji :

 Hi all,

 Could someone please review and sponsor this fix ?
 I write the details of this issue again. Please review it.

 =Problem=
 Potential infinite waiting at TCPChannel#createConnection.

 This method flushes the DataOutputStream without the socket
 timeout settings when choose stream protocol [1]. If connection lost
 or the destination server do not return response during the flush,
 this method wait forever because the timeout settings is set the
 default value of SO_TIMEOUT, i.e., infinite.

 [1]:
 http://hg.openjdk.java.net/jdk9/dev/jdk/file/7adef1c3afd5/src/java.rmi/share/classes/sun/rmi/transport/tcp/TCPChannel.java#l227

 I think this issue is rarely, however serious.

 =Reproduce=
 I write a test program to reproduce. You can reproduce by the below.

 * hg clone
 http://icedtea.classpath.org/people/ykubota/fixLoopAtJMXConnectorFactory/
 * cd fixLoopAtJMXConnector

Re: RFR [7] 8133206: "java.lang.OutOfMemoryError: unable to create new native thread" caused by upgrade to zlib 1.2.8

[Andrew Brygin]

I’d like to cast a vote for inclusion this fix in jdk9.

Probably it has to be done in the original review thread created by Alex:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036463.html
http://mail.openjdk.java.net/pipermail/jdk9-dev/2015-November/003036.html
but there is no any activity from November 2015…

So, +1 to get this fix in jdk9.

Thanks,
Andrew


On Feb 24, 2016, at 5:07 PM, Dmitry Cherepanov 
mailto:dcherepa...@azul.com>> wrote:


On Feb 19, 2016, at 1:55 PM, Nikolay Gorshkov 
mailto:niko...@azulsystems.com>> wrote:

Hi Sherman, Sean,

Could you please help with making progress on this code review request?
This fix is waiting for review since October.

Webrev for jdk7u:
http://cr.openjdk.java.net/~nikgor/8133206/jdk7u-dev/webrev.01/
Original mail thread:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-October/035884.html

I’m not expert in this area but the changes look reasonable to me. +1 for 
pushing this into JDK9.

Thanks

Dmitry


Webrev for jdk9 (contributed by Alex Kashchenko):
http://cr.openjdk.java.net/~akasko/jdk9/8133206/webrev.00/
Original mail thread:
http://mail.openjdk.java.net/pipermail/jdk9-dev/2015-November/003036.html

Thanks,
Nikolay

On 11.12.2015 15:09, Nikolay Gorshkov wrote:
Hi Sean,

Thank you for your attention to this matter!
Actually, the code review request was sent to core-libs-dev alias a month ago:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036463.html
Unfortunately, we haven't got any feedback yet.

Thanks,
Nikolay

On 11.12.2015 14:34, Seán Coffey wrote:
Hi Alex,

I'm dropping jdk7u-dev mailing list for the moment. core-libs-dev is the
mailing list where this review should happen. This fix would be required in JDK
9 first as per process. I think Sherman would be best to review if possible.

Once it's soaked in JDK 9 for a few weeks, we could consider jdk8u and 7u
backports.

Regards,
Sean.

On 10/11/15 10:57, Alex Kashchenko wrote:
Hi,

On 10/21/2015 04:39 PM, Nikolay Gorshkov wrote:
Hi Sherman,

Thank you for your reply! My answers are inlined.

> Can you be more specific about the "class loading cases" above? Sounds
> more like we have a memory leaking here (the real root cause)? for
example
> the inflateEnd() never gets called?

I agree, the real root cause is probably the following issue that exists
since the end of 2002:
https://bugs.openjdk.java.net/browse/JDK-4797189
"Finalizers not called promptly enough"
And it is "the absence of a general solution to the non-heap resource
exhaustion problem".

zlib's inflateEnd() function is called by
void java.util.zip.Inflater.end(long addr)
native method only, and this method, in turn, is called only by
void java.util.zip.Inflater.end()
and
void java.util.zip.Inflater.finalize()
methods. According to the experiments, the typical stack trace for
instantiating java.util.zip.Inflater is:

java.util.zip.Inflater.(Inflater.java:116)
java.util.zip.ZipFile.getInflater(ZipFile.java:450)
java.util.zip.ZipFile.getInputStream(ZipFile.java:369)
java.util.jar.JarFile.getInputStream(JarFile.java:412)
org.jboss.virtual.plugins.context.zip.ZipFileWrapper.openStream(ZipFileWrapper.java:222)



  
org.jboss.classloader.spi.base.BaseClassLoader$2.run(BaseClassLoader.java:592)

java.security.AccessController.doPrivileged(Native Method)
org.jboss.classloader.spi.base.BaseClassLoader.loadClassLocally(BaseClassLoader.java:591)



  
org.jboss.classloader.spi.base.BaseClassLoader.loadClass(BaseClassLoader.java:447)



java.lang.ClassLoader.loadClass(ClassLoader.java:358)
java.lang.Class.forName0(Native Method)
java.lang.Class.forName(Class.java:278)
org.jboss.deployers.plugins.annotations.WeakClassLoaderHolder.loadClass(WeakClassLoaderHolder.java:72)



  

It's quite hard to understand who is responsible for not calling
Inflater.end()
method explicitly; probably, it is the jboss/application's code.
Unfortunately,
we were in "it worked before and is broken now" customer situation here, so
needed to fix it anyway.

>  From the doc/impl in inflate() it appears the proposed change should be
> fine, though it's a little hacky, as you never know if it starts to
return
> Z_OK from some future release(s). Since the "current" implementation
> never returns Z_OK, it might be worth considering to keep the Z_OK logic
> asis in Inflater.c, together with the Z_BUF_ERROR, just in case?

OK, I added handling of Z_OK code back.

> I would be desired to add some words in Inflater.c to remind the
> future maintainer why we switched from partial to finish and why to
> check z_buf_error.

I agree, added a comment.

The updated webrev is available here:

http://cr.openjdk.java.net/~nikgor/8133206/jdk7u-dev/webrev.01/


The change looks good to me (not a Reviewer/Committer).

Patched jdk7u also passes JCK-7 on RHEL 7.1.

I forward-ported this patch to jdk9 (consulted with Nikolay Gorshkov first),
jtreg reproducer for jdk9 also works with jdk7u -
http://mail.openjdk.java.net/pipermail/jdk9-dev/2015-Nove

Re: Custom security policy without replacing files in the OpenJDK?

[Sean Mullan]

On 03/02/2016 08:45 PM, David Holmes wrote:

On 27/02/2016 2:56 AM, Marcus Lagergren wrote:

Hi!

Is it possible to override lib/security/local_policy on app level
without patching jdk distro?
i.e. -Duse.this.policy.jar= … or something?

Can’t find a way to do it


http://docs.oracle.com/javase/8/docs/technotes/guides/security/PolicyFiles.html


Specifying an Additional Policy File at Runtime

It is also possible to specify an additional or a different policy file
when invoking execution of an application. This can be done via the
"-Djava.security.policy" command line argument, which sets the value of
the java.security.policy property. For example, if you use

 java -Djava.security.manager -Djava.security.policy=someURL SomeApp


I believe Marcus is referring to local_policy.jar which is for enforcing 
import restrictions on cryptography.


This is not a security policy file so the above command line won't work.

There is no way to override it from the command line AFAIK.

Thanks,
Sean

Re: [8u-dev] Request for REVIEW and APPROVAL to backport: 8149330: Capacity of StringBuilder should not get close to Integer.MAX_VALUE unless necessary

[Ivan Gerasimov]

Thank you Martin and Seán!

I'll add some info to the bug report with a reproducer code and symptoms.

Sincerely yours,
Ivan

On 03.03.2016 12:07, Seán Coffey wrote:

Approved for jdk8u-dev (BTW).

Regards,
Sean.

On 03/03/2016 09:04, Seán Coffey wrote:

Ivan,

the JBS bug description is scare on detail. Can you fill it out a bit ?

Some examples of the stack trace encountered and links to OpenJDK 
reviews/discussions will help people who encounter this issue in the 
future.


Regards,
Sean.

On 02/03/2016 20:20, Martin Buchholz wrote:

Reviewed!

On Wed, Mar 2, 2016 at 9:29 AM, Ivan Gerasimov
 wrote:

Hello!

I'm seeking for approval to backport this fix into jdk8u-dev.
Comparing to Jdk9, the patch had to be changed mainly due to 
compact string

support introduced in jdk9.
However, the fix is essentially the same: we just avoid getting too 
close to

Integer.MAX_VALUE when doing reallocations unless explicitly required.

Would you please help review it?

Bug: https://bugs.openjdk.java.net/browse/JDK-8149330
Jdk9 change: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/123593aacb48
Jdk9 review:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-February/039018.html 

http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-March/039182.html 


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

Sincerely yours,
Ivan

Re: RFR(S): 8150957: j.l.i.MethodHandles.whileLoop(...) fails with IOOBE in the case 'init' is null, 'step' and 'pred' have parameters

[Michael Haupt]

Hi Paul,

> Am 03.03.2016 um 11:58 schrieb Paul Sandoz :
> Minor comment, up to you to accept/reject: you could assert that “w.i” is the 
> expected value after the loop invocation.

thank you. Excellent suggestion, I'll push with that added.

Best,

Michael

-- 

 
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
Oracle Java Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG | Schiffbauergasse 14 | 14467 Potsdam, Germany

ORACLE Deutschland B.V. & Co. KG | Hauptverwaltung: Riesstraße 25, D-80992 
München
Registergericht: Amtsgericht München, HRA 95603

Komplementärin: ORACLE Deutschland Verwaltung B.V. | Hertogswetering 163/167, 
3543 AS Utrecht, Niederlande
Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher
  Oracle is committed to developing 
practices and products that help protect the environment

Re: RFR: jsr166 jdk9 integration wave 5

[Paul Sandoz]

> On 2 Mar 2016, at 21:37, Martin Buchholz  wrote:
> 
> Webrevs updated, incorporating changes to tests in my previous message.

Looks ok, but i went through rather quickly.

java/util/concurrent/ScheduledThreadPoolExecutor/DelayOverflow.java
—

-pool.schedule(keepPoolBusy, 0, TimeUnit.SECONDS);
+pool.schedule(keepPoolBusy, 0, DAYS);

It probably does not matter that you changed the units here?

Paul.

Re: RFR 8150679: closed/javax/crypto/CryptoPermission/CallerIdentification.sh fails after fix for JDK-8132734

[Paul Sandoz]

> On 2 Mar 2016, at 20:12, Steve Drach  wrote:
> 
> Please review the following fix for JDK-8150679
> 
> webrev: http://cr.openjdk.java.net/~sdrach/8150679/webrev/ 
> 
> issue: https://bugs.openjdk.java.net/browse/JDK-8150679 
> 
> 
> The test was modified to demonstrate the problem.

You are essentially bombing out of MR-JAR functionality if the JarEntry is not 
an instance of JarFileEntry. That might be ok for a short-term solution, but it 
might require some further deeper investigation on things that extend JarEntry 
and how it is is used by VerifierStream [*].

JarFile:

 895 private JarEntry verifiableEntry(ZipEntry ze) {
 896 if (ze == null) return null;

You don’t need this. The code will anyway throw an NPE elsewhere, and the 
original code threw an NPE when obtaining the name:

return new JarVerifier.VerifierStream(
getManifestFromReference(),
ze instanceof JarFileEntry ?
(JarEntry) ze : getJarEntry(ze.getName()),
super.getInputStream(ze),
jv);


 897 if (ze instanceof JarFileEntry) {
 898 // assure the name and entry match for verification
 899 return ((JarFileEntry)ze).reifiedEntry();
 900 }
 901 ze = getJarEntry(ze.getName());
 902 assert ze instanceof JarEntry;

This assertion is redundant as the method signature of getJarEntry returns 
JarEntry.


 903 if (ze instanceof JarFileEntry) {
 904 return ((JarFileEntry)ze).reifiedEntry();
 905 }
 906 return (JarEntry)ze;
 907 }


MultiReleaseJarURLConnection
—

Given your changes above i am confused how your test passes for instances of 
URLJarFileEntry since they cannot be reified.

Paul.

[*] AFAICT JarVerifier directly accesses the fields JarEntry.signers/certs.

Re: RFR(S): 8150957: j.l.i.MethodHandles.whileLoop(...) fails with IOOBE in the case 'init' is null, 'step' and 'pred' have parameters

[Paul Sandoz]

> On 2 Mar 2016, at 20:53, Michael Haupt  wrote:
> 
> Dear all,
> 
> please review this change.
> Bug: https://bugs.openjdk.java.net/browse/JDK-8150957
> Webrev: http://cr.openjdk.java.net/~mhaupt/8150957/webrev.00/
> 
> The bug was actually fixed with the push for JDK-8150635. This change adds a 
> test for the issue.
> 

Looks good.

Minor comment, up to you to accept/reject: you could assert that “w.i” is the 
expected value after the loop invocation.

Paul.

Re: RFR [9] 8151140: Replace use of lambda/method ref in jdk.Version constructor

[Paul Sandoz]

+1

Paul.

> On 3 Mar 2016, at 11:31, Chris Hegarty  wrote:
> 
> Since 8150163 [1], jdk.Version can now be used earlier in startup, but not
> always. It was noticed that the use of lambda / method ref in the constructor,
> in some cases, was the first usage of such, and incurred the initialization
> costs of the java.lang.invoke infrastructure ( which can take a significant
> amount of time on first access).
> 
> The solution is to simple avoid the usage, as has been done in other “core"
> areas, that may be used early in startup.
> 
> diff --git a/src/java.base/share/classes/jdk/Version.java 
> b/src/java.base/share/classes/jdk/Version.java
> --- a/src/java.base/share/classes/jdk/Version.java
> +++ b/src/java.base/share/classes/jdk/Version.java
> @@ -28,10 +28,10 @@
> import java.math.BigInteger;
> import java.security.AccessController;
> import java.security.PrivilegedAction;
> +import java.util.ArrayList;
> import java.util.regex.Matcher;
> import java.util.regex.Pattern;
> import java.util.stream.Collectors;
> -import java.util.Arrays;
> import java.util.Collections;
> import java.util.List;
> import java.util.Optional;
> @@ -208,11 +208,10 @@
>+ s + "'");
> 
> // $VNUM is a dot-separated list of integers of arbitrary length
> -version
> -= Collections.unmodifiableList(
> -  Arrays.stream(m.group(VNUM_GROUP).split("\\."))
> -  .map(Integer::parseInt)
> -  .collect(Collectors.toList()));
> +List list = new ArrayList<>();
> +for (String i : m.group(VNUM_GROUP).split("\\."))
> +list.add(Integer.parseInt(i));
> +version = Collections.unmodifiableList(list);
> 
> pre = Optional.ofNullable(m.group(PRE_GROUP));
> 
> -Chris.
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8150976
> 
> 

Re: RFR: 8151123 - Collectors.summingDouble/averagingDouble unnecessarily call mapper twice

[Paul Sandoz]

> On 3 Mar 2016, at 10:26, Tagir F. Valeev  wrote:
> 
> Hello!
> 
> Please review and sponsor this small change:
> 
> https://bugs.openjdk.java.net/browse/JDK-8151123
> http://cr.openjdk.java.net/~tvaleev/webrev/8151123/r1/
> 
> User-supplied mapper function is unnecessarily called twice on each
> accumulation event in summingDouble and averagingDouble. This function
> could be computationally intensive which may degrade the performance
> up to 2x. The patch addresses this issue.
> 

+1 An embarrassing oversight missed in review, well spotted. I can push for you.

—

I find it annoying we have to maintain the compensated and uncompensated sum 
for the edge case of the compensated sum being NaN and the simple sum being 
infinite. I measured this a while back i was surprised it did not appear to 
make much difference when loops are unrolled and vectored instructions are 
used, but i did perform an in-depth investigation.

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


Paul.


> Here's also simple JMH benchmark which illustrates the performance
> gain.
> http://cr.openjdk.java.net/~tvaleev/webrev/8151123/jmh/
> 
> Original:
> 
> Benchmark (n)  Mode  Cnt ScoreError  Units
> AveragingTest.averageDistance  10  avgt   30 0,571 ±  0,049  us/op
> AveragingTest.averageDistance1000  avgt   3058,573 ±  1,194  us/op
> AveragingTest.averageDistance  10  avgt   30  5854,428 ± 71,242  us/op
> 
> Patched:
> 
> Benchmark (n)  Mode  Cnt ScoreError  Units
> AveragingTest.averageDistance  10  avgt   30 0,336 ±  0,002  us/op
> AveragingTest.averageDistance1000  avgt   3031,932 ±  0,367  us/op
> AveragingTest.averageDistance  10  avgt   30  3794,541 ± 21,599  us/op
> 
> With best regards,
> Tagir Valeev.
> 

RFR [9] 8151140: Replace use of lambda/method ref in jdk.Version constructor

[Chris Hegarty]

Since 8150163 [1], jdk.Version can now be used earlier in startup, but not
always. It was noticed that the use of lambda / method ref in the constructor,
in some cases, was the first usage of such, and incurred the initialization
costs of the java.lang.invoke infrastructure ( which can take a significant
amount of time on first access). 

The solution is to simple avoid the usage, as has been done in other “core"
areas, that may be used early in startup.

diff --git a/src/java.base/share/classes/jdk/Version.java 
b/src/java.base/share/classes/jdk/Version.java
--- a/src/java.base/share/classes/jdk/Version.java
+++ b/src/java.base/share/classes/jdk/Version.java
@@ -28,10 +28,10 @@
 import java.math.BigInteger;
 import java.security.AccessController;
 import java.security.PrivilegedAction;
+import java.util.ArrayList;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
-import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
 import java.util.Optional;
@@ -208,11 +208,10 @@
+ s + "'");
 
 // $VNUM is a dot-separated list of integers of arbitrary length
-version
-= Collections.unmodifiableList(
-  Arrays.stream(m.group(VNUM_GROUP).split("\\."))
-  .map(Integer::parseInt)
-  .collect(Collectors.toList()));
+List list = new ArrayList<>();
+for (String i : m.group(VNUM_GROUP).split("\\."))
+list.add(Integer.parseInt(i));
+version = Collections.unmodifiableList(list);
 
 pre = Optional.ofNullable(m.group(PRE_GROUP));

-Chris.

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

Re: RFR(s): 8150460: (linux|bsd|aix)_close.c: file descriptor table may become large or may not work at all

[Dmitry Samersoff]

Thomas,

> The more I look at this, the more I think that the costs for a
> pthread mutex lock are acceptable in this case: we are about to do a
> blocking IO operation anyway, which is already flanked by two mutex
> locking calls (in startOp and endOp). I doubt that a third mutex call
> will be the one making the costs suddenly unacceptable. Especially
> since they can be avoided altogether for low value mutex numbers (the
> optimization Roger suggested).

After closer look to the code in a whole - I agree with you.

-Dmitry


On 2016-03-03 12:32, Thomas Stüfe wrote:
> Hi Hans,
> 
> On Thu, Mar 3, 2016 at 4:08 AM, Hans Boehm  > wrote:
> 
> 
> On Wed, Mar 2, 2016 at 12:09 AM, Thomas Stüfe 
> mailto:thomas.stu...@gmail.com>> wrote:
>> 
>> Hi Hans,
>> 
>> thanks for the hint!
>> 
>> But how would I do this for my problem:
>> 
>> Allocate memory, zero it out and then store the pointer into a
>> variable seen by other threads, while preventing the other threads
>> from seeing . I do not understand how atomics would help: I can
>> make the pointer itself an atomic, but that only guarantees memory
>> ordering in regard to this variable, not to the allocated memory.
>> 
>> Kind Regards, Thomas
> 
> C11 atomics work essentially like Java volatiles: They order other 
> memory accesses as well.  If you declare the pointer to be atomic, 
> and store into it, then another thread reading the newly assigned 
> value will also see the stores preceding the pointer store.  Since 
> the pointer is the only value that can be accessed concurrently by 
> multiple threads (with not all accesses reads), it's the only object 
> that needs to be atomic.  In this case, it's sufficient to store into
> the pointer with
> 
> atomic_store_explicit(&ptr, , memory_order_release);
> 
> and read it with
> 
> atomic_load_explicit(&ptr, memory_order_acquire);
> 
> which are a bit cheaper.
> 
> 
> However, this is C11 specific, and I don't know whether that's 
> acceptable to use in this context.
> 
> If you can't assume C11, the least incorrect workaround is generally 
> to make the pointer volatile, precede the store with a fence, and 
> follow the load with a fence.  On x86, both fences just need to 
> prevent compiler reordering.
> 
> 
> 
> Thank you for that excellent explanation!
> 
> This may be just my ignorance, but I actually did not know that
> atomics are now a part of the C standard. I took this occasion to
> look up all other C11 features and this is quite neat :) Nice to see
> that C continues to live.
> 
> I am very hesitant though about introducing C11 features into the
> JDK. We deal with notoriously oldish compilers, especially on AIX,
> and I do not want to be the first to force C11, especially not on
> such a side issue.
> 
> The more I look at this, the more I think that the costs for a
> pthread mutex lock are acceptable in this case: we are about to do a
> blocking IO operation anyway, which is already flanked by two mutex
> locking calls (in startOp and endOp). I doubt that a third mutex call
> will be the one making the costs suddenly unacceptable. Especially
> since they can be avoided altogether for low value mutex numbers (the
> optimization Roger suggested).
> 
> I will do some performance tests and check whether the added locking 
> calls are even measurable.
> 
> Thomas
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.

Re: RFR(s): 8150460: (linux|bsd|aix)_close.c: file descriptor table may become large or may not work at all

[Thomas Stüfe]

Hi Hans,

On Thu, Mar 3, 2016 at 4:08 AM, Hans Boehm  wrote:

>
> On Wed, Mar 2, 2016 at 12:09 AM, Thomas Stüfe 
> wrote:
> >
> > Hi Hans,
> >
> > thanks for the hint!
> >
> > But how would I do this for my problem:
> >
> > Allocate memory, zero it out and then store the pointer into a variable
> seen by other threads, while preventing the other threads from seeing . I
> do not understand how atomics would help: I can make the pointer itself an
> atomic, but that only guarantees memory ordering in regard to this
> variable, not to the allocated memory.
> >
> > Kind Regards, Thomas
>
> C11 atomics work essentially like Java volatiles: They order other memory
> accesses as well.  If you declare the pointer to be atomic, and store into
> it, then another thread reading the newly assigned value will also see the
> stores preceding the pointer store.  Since the pointer is the only value
> that can be accessed concurrently by multiple threads (with not all
> accesses reads), it's the only object that needs to be atomic.  In this
> case, it's sufficient to store into the pointer with
>
> atomic_store_explicit(&ptr, , memory_order_release);
>
> and read it with
>
> atomic_load_explicit(&ptr, memory_order_acquire);
>
> which are a bit cheaper.
>
>
However, this is C11 specific, and I don't know whether that's acceptable
> to use in this context.
>
> If you can't assume C11, the least incorrect workaround is generally to
> make the pointer volatile, precede the store with a fence, and follow the
> load with a fence.  On x86, both fences just need to prevent compiler
> reordering.
>


Thank you for that excellent explanation!

This may be just my ignorance, but I actually did not know that atomics are
now a part of the C standard. I took this occasion to look up all other C11
features and this is quite neat :) Nice to see that C continues to live.

I am very hesitant though about introducing C11 features into the JDK. We
deal with notoriously oldish compilers, especially on AIX, and I do not
want to be the first to force C11, especially not on such a side issue.

The more I look at this, the more I think that the costs for a pthread
mutex lock are acceptable in this case: we are about to do a blocking IO
operation anyway, which is already flanked by two mutex locking calls (in
startOp and endOp). I doubt that a third mutex call will be the one making
the costs suddenly unacceptable. Especially since they can be avoided
altogether for low value mutex numbers (the optimization Roger suggested).

I will do some performance tests and check whether the added locking calls
are even measurable.

Thomas

RFR: 8151123 - Collectors.summingDouble/averagingDouble unnecessarily call mapper twice

[Tagir F. Valeev]

Hello!

Please review and sponsor this small change:

https://bugs.openjdk.java.net/browse/JDK-8151123
http://cr.openjdk.java.net/~tvaleev/webrev/8151123/r1/

User-supplied mapper function is unnecessarily called twice on each
accumulation event in summingDouble and averagingDouble. This function
could be computationally intensive which may degrade the performance
up to 2x. The patch addresses this issue.

Here's also simple JMH benchmark which illustrates the performance
gain.
http://cr.openjdk.java.net/~tvaleev/webrev/8151123/jmh/

Original:

Benchmark (n)  Mode  Cnt ScoreError  Units
AveragingTest.averageDistance  10  avgt   30 0,571 ±  0,049  us/op
AveragingTest.averageDistance1000  avgt   3058,573 ±  1,194  us/op
AveragingTest.averageDistance  10  avgt   30  5854,428 ± 71,242  us/op

Patched:

Benchmark (n)  Mode  Cnt ScoreError  Units
AveragingTest.averageDistance  10  avgt   30 0,336 ±  0,002  us/op
AveragingTest.averageDistance1000  avgt   3031,932 ±  0,367  us/op
AveragingTest.averageDistance  10  avgt   30  3794,541 ± 21,599  us/op

With best regards,
Tagir Valeev.

Re: [8u-dev] Request for REVIEW and APPROVAL to backport: 8149330: Capacity of StringBuilder should not get close to Integer.MAX_VALUE unless necessary

[Seán Coffey]

Approved for jdk8u-dev (BTW).

Regards,
Sean.

On 03/03/2016 09:04, Seán Coffey wrote:

Ivan,

the JBS bug description is scare on detail. Can you fill it out a bit ?

Some examples of the stack trace encountered and links to OpenJDK 
reviews/discussions will help people who encounter this issue in the 
future.


Regards,
Sean.

On 02/03/2016 20:20, Martin Buchholz wrote:

Reviewed!

On Wed, Mar 2, 2016 at 9:29 AM, Ivan Gerasimov
 wrote:

Hello!

I'm seeking for approval to backport this fix into jdk8u-dev.
Comparing to Jdk9, the patch had to be changed mainly due to compact 
string

support introduced in jdk9.
However, the fix is essentially the same: we just avoid getting too 
close to

Integer.MAX_VALUE when doing reallocations unless explicitly required.

Would you please help review it?

Bug: https://bugs.openjdk.java.net/browse/JDK-8149330
Jdk9 change: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/123593aacb48
Jdk9 review:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-February/039018.html 

http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-March/039182.html 


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

Sincerely yours,
Ivan

Re: [8u-dev] Request for REVIEW and APPROVAL to backport: 8149330: Capacity of StringBuilder should not get close to Integer.MAX_VALUE unless necessary

[Seán Coffey]

Ivan,

the JBS bug description is scare on detail. Can you fill it out a bit ?

Some examples of the stack trace encountered and links to OpenJDK 
reviews/discussions will help people who encounter this issue in the future.


Regards,
Sean.

On 02/03/2016 20:20, Martin Buchholz wrote:

Reviewed!

On Wed, Mar 2, 2016 at 9:29 AM, Ivan Gerasimov
 wrote:

Hello!

I'm seeking for approval to backport this fix into jdk8u-dev.
Comparing to Jdk9, the patch had to be changed mainly due to compact string
support introduced in jdk9.
However, the fix is essentially the same: we just avoid getting too close to
Integer.MAX_VALUE when doing reallocations unless explicitly required.

Would you please help review it?

Bug: https://bugs.openjdk.java.net/browse/JDK-8149330
Jdk9 change: http://hg.openjdk.java.net/jdk9/dev/jdk/rev/123593aacb48
Jdk9 review:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-February/039018.html
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-March/039182.html
Jdk8 webrev: http://cr.openjdk.java.net/~igerasim/8149330/04/webrev/

Sincerely yours,
Ivan