RE: RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]

2018-03-27 Thread Langer, Christoph
Hi Ivan,

I went through your changes and I really like this work.

Nothing to complain about - especially if there are no regressions with the 
additional test coverage.

Best regards
Christoph


> -Original Message-
> From: net-dev [mailto:net-dev-boun...@openjdk.java.net] On Behalf Of
> Ivan Gerasimov
> Sent: Dienstag, 27. März 2018 00:08
> To: Chris Hegarty ; net-dev@openjdk.java.net;
> Alan Bateman 
> Subject: Re: RFR [11] 8198358 : Align organization of
> DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]
> 
> Hello everyone!
> 
> A few modifications were done to the patch.
> Below is the list of updated parts of the patch with comments about what
> has changed, comparing to the previous version of the patch.
> 
> The patched JDK builds fine and all the tests pass well.
> 
> WEBREV-000:
> http://cr.openjdk.java.net/~igerasim/8198358/04/000/webrev/
> WEBREV-001:
> http://cr.openjdk.java.net/~igerasim/8198358/04/001/webrev/
> WEBREV-002:
> http://cr.openjdk.java.net/~igerasim/8198358/04/002/webrev/
> WEBREV-003:
> http://cr.openjdk.java.net/~igerasim/8198358/04/003/webrev/
> - Rebased to reflect recent changes to socketListen()
> 
> WEBREV-004:
> http://cr.openjdk.java.net/~igerasim/8198358/04/004/webrev/
> WEBREV-005:
> http://cr.openjdk.java.net/~igerasim/8198358/04/005/webrev/
> WEBREV-006:
> http://cr.openjdk.java.net/~igerasim/8198358/04/006/webrev/
> WEBREV-007:
> http://cr.openjdk.java.net/~igerasim/8198358/04/007/webrev/
> WEBREV-008:
> http://cr.openjdk.java.net/~igerasim/8198358/04/008/webrev/
> - The check the line 400 was changed to test if (newfd < 0) and then if
> (newfd == -2).
> I don't think the later is quite accurate (cannot find in the
> documentation that accept can return -2), but it is how it was done in
> TwoStacks originally, so no regression is expected.
> 
> WEBREV-009:
> http://cr.openjdk.java.net/~igerasim/8198358/04/009/webrev/
> - Added missing 'return -1;' at the line 188.
> 
> WEBREV-010:
> http://cr.openjdk.java.net/~igerasim/8198358/04/010/webrev/
> WEBREV-011:
> http://cr.openjdk.java.net/~igerasim/8198358/04/011/webrev/
> WEBREV-012:
> http://cr.openjdk.java.net/~igerasim/8198358/04/012/webrev/
> WEBREV-013:
> http://cr.openjdk.java.net/~igerasim/8198358/04/013/webrev/
> WEBREV-014:
> http://cr.openjdk.java.net/~igerasim/8198358/04/014/webrev/
> 
> With kind regards,
> Ivan



-Djava.security.manager=problems for service providers

2018-03-27 Thread Peter Firmstone

Not sure if this is the right place to mention this.

Anyone notice that specifying a custom security manager at jvm start up 
causes issues with service providers loading?   If using the sun 
PolicyFile implementation, the policy doesn't load due to the provider 
failure, I have a custom policy implementation that will allow the jvm 
to run in this state, and other providers are also not loading, such as 
the logger and JCE.


Note that it doesn't occur if the security manager is set 
programmatically in the main method at start up, only if it's set via 
command line option.


Examples of providers not loading:

 [java] java.lang.NullPointerException
 [java] Can't load log handler "java.util.logging.ConsoleHandler"
 [java] java.lang.NullPointerException
 [java] java.lang.NullPointerException
 [java] at java.util.logging.LogManager$5.run(LogManager.java:965)
 [java] at java.security.AccessController.doPrivileged(Native 
Method)
 [java] at 
java.util.logging.LogManager.loadLoggerHandlers(LogManager.java:958)
 [java] at 
java.util.logging.LogManager.initializeGlobalHandlers(LogManager.java:1578)
 [java] at 
java.util.logging.LogManager.access$1500(LogManager.java:145)
 [java] at 
java.util.logging.LogManager$RootLogger.accessCheckedHandlers(LogManager.java:1667)

 [java] at java.util.logging.Logger.getHandlers(Logger.java:1777)
 [java] at java.util.logging.Logger.log(Logger.java:735)
 [java] at java.util.logging.Logger.doLog(Logger.java:765)
 [java] at java.util.logging.Logger.log(Logger.java:788)
 [java] at 
org.apache.river.api.security.ConcurrentPolicyFile$2.run(ConcurrentPolicyFile.java:496)
 [java] at 
org.apache.river.api.security.ConcurrentPolicyFile$2.run(ConcurrentPolicyFile.java:469)
 [java] at java.security.AccessController.doPrivileged(Native 
Method)
 [java] at 
org.apache.river.api.security.ConcurrentPolicyFile.readPoliciesNoCheckGuard(ConcurrentPolicyFile.java:468)
 [java] at 
org.apache.river.api.security.ConcurrentPolicyFile.readPolicyPermissionGrants(ConcurrentPolicyFile.java:243)
 [java] at 
org.apache.river.api.security.ConcurrentPolicyFile.(ConcurrentPolicyFile.java:253)
 [java] at 
org.apache.river.api.security.ConcurrentPolicyFile.(ConcurrentPolicyFile.java:226)
 [java] at 
org.apache.river.api.security.CombinerSecurityManager.(CombinerSecurityManager.java:154)
 [java] at 
org.apache.river.api.security.CombinerSecurityManager.(CombinerSecurityManager.java:133)
 [java] at 
org.apache.river.tool.SecurityPolicyWriter.(SecurityPolicyWriter.java:137)
 [java] at 
org.apache.river.tool.SecurityPolicyWriter.(SecurityPolicyWriter.java:162)
 [java] at 
sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
 [java] at 
sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
 [java] at 
sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
 [java] at 
java.lang.reflect.Constructor.newInstance(Constructor.java:423)

 [java] at java.lang.Class.newInstance(Class.java:442)
 [java] at sun.misc.Launcher.(Launcher.java:93)
 [java] at sun.misc.Launcher.(Launcher.java:54)
 [java] at 
java.lang.ClassLoader.initSystemClassLoader(ClassLoader.java:1451)
 [java] at 
java.lang.ClassLoader.getSystemClassLoader(ClassLoader.java:1436)



 [java] Error occurred during initialization of VM
 [java] java.lang.ExceptionInInitializerError
 [java] at 
java.util.ResourceBundle.getLoader(ResourceBundle.java:482)
 [java] at 
java.util.ResourceBundle.getBundle(ResourceBundle.java:783)
 [java] at 
sun.security.util.ResourcesMgr$1.run(ResourcesMgr.java:47)
 [java] at 
sun.security.util.ResourcesMgr$1.run(ResourcesMgr.java:44)
 [java] at java.security.AccessController.doPrivileged(Native 
Method)
 [java] at 
sun.security.util.ResourcesMgr.getString(ResourcesMgr.java:43)
 [java] at 
sun.security.provider.PolicyFile.addGrantEntry(PolicyFile.java:888)
 [java] at 
sun.security.provider.PolicyFile.init(PolicyFile.java:626)
 [java] at 
sun.security.provider.PolicyFile.access$400(PolicyFile.java:258)
 [java] at 
sun.security.provider.PolicyFile$3.run(PolicyFile.java:521)
 [java] at 
sun.security.provider.PolicyFile$3.run(PolicyFile.java:495)
 [java] at java.security.AccessController.doPrivileged(Native 
Method)
 [java] at 
sun.security.provider.PolicyFile.initPolicyFile(PolicyFile.java:495)
 [java] at 
sun.security.provider.PolicyFile.initPolicyFile(PolicyFile.java:480)
 [java] at 
sun.security.provider.PolicyFile.init(PolicyFile.java:439)
 [java] at 
sun.security.provider.PolicyFile.(PolicyFile.java:297)

 [java] at java.security.Policy.getPoli

Re: RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]

2018-03-27 Thread Chris Hegarty
Ivan,

> On 26 Mar 2018, at 23:08, Ivan Gerasimov  wrote:
> 
> Hello everyone!
> 
> A few modifications were done to the patch.
> Below is the list of updated parts of the patch with comments about what has 
> changed, comparing to the previous version of the patch.
> 
> The patched JDK builds fine and all the tests pass well.
> 
> WEBREV-000: http://cr.openjdk.java.net/~igerasim/8198358/04/000/webrev/

Ok.

> WEBREV-001: http://cr.openjdk.java.net/~igerasim/8198358/04/001/webrev/

Ok.

> WEBREV-002: http://cr.openjdk.java.net/~igerasim/8198358/04/002/webrev/

Can we make fdAccess final ( and in DualStack too )
static *final* JavaIOFileDescriptorAccess fdAccess

JNIEXPORT void JNICALL Java_java_net_TwoStacksPlainSocketImpl_bind0 ...

 142 if (IS_NULL(iaObj)) {
 143 JNU_ThrowNullPointerException(env, "inet address argument");
 144 return;
 145 }
 146 
 147 family = getInetAddress_family(env, iaObj);
 148 if (family != java_net_InetAddress_IPv4) {
 149 JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException",
 150 "Protocol family not supported");
 151 return;
 152 }

The address is already null checked at the java level, so can be removed here, 
no?

Can the family check be hoisted up into Java? I think this will help when it 
comes
to the eventual merge with DualStack.

The native code will then align with DualStack’s.  I prefer to reduce and align
the native layers as much as possible. It’s easier to deal with differences at
the Java level.

> WEBREV-003: http://cr.openjdk.java.net/~igerasim/8198358/04/003/webrev/
> - Rebased to reflect recent changes to socketListen()

Ok.

> WEBREV-004: http://cr.openjdk.java.net/~igerasim/8198358/04/004/webrev/

Ok.

> WEBREV-005: http://cr.openjdk.java.net/~igerasim/8198358/04/005/webrev/

Ok.

> WEBREV-006: http://cr.openjdk.java.net/~igerasim/8198358/04/006/webrev/

Ok.

> WEBREV-007: http://cr.openjdk.java.net/~igerasim/8198358/04/007/webrev/

Ok.

> WEBREV-008: http://cr.openjdk.java.net/~igerasim/8198358/04/008/webrev/
> - The check the line 400 was changed to test if (newfd < 0) and then if 
> (newfd == -2).
> I don't think the later is quite accurate (cannot find in the documentation 
> that accept can return -2), but it is how it was done in TwoStacks 
> originally, so no regression is expected.

Ok.

 413 if (sa.sa.sa_family != AF_INET) {
 414 JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException",
 415 "Protocol family not supported");
 416 NET_SocketClose(newfd);
 417 return -1;
 418 }

This check could be hoisted up to the Java-level too, no? Again, I think it
would be easier to deal with this kinda differences in Java.

> WEBREV-009: http://cr.openjdk.java.net/~igerasim/8198358/04/009/webrev/
> - Added missing 'return -1;' at the line 188.

Can the family be checked at the java level.

> WEBREV-010: http://cr.openjdk.java.net/~igerasim/8198358/04/010/webrev/

Ok.

> WEBREV-011: http://cr.openjdk.java.net/~igerasim/8198358/04/011/webrev/

Ok.

> WEBREV-012: http://cr.openjdk.java.net/~igerasim/8198358/04/012/webrev/

Ok.

> WEBREV-013: http://cr.openjdk.java.net/~igerasim/8198358/04/013/webrev/

Ok.

> WEBREV-014: http://cr.openjdk.java.net/~igerasim/8198358/04/014/webrev/

Good.

-Chris.



Re: RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]

2018-03-27 Thread Ivan Gerasimov

Thank you Christoph and Chris for the review!

I made the following suggested changes:
- fdAccess made private static final in both TwoStacks and DualStack.
- removed redundant check IS_NULL(iaObj) at bind0 (indeed, the address 
was already checked at Java level).


For now, I would prefer to keep the checks for family == IPv4 as they are.
I totally agree they should be moved to Java, but I'm planning to do 
this during the TwoStacks-DualStack merge step.


I'm also planning to add a regtest to make sure that Inet6Address is 
rejected when fed to a IPv4-only socked.


Are we good to push now?
Please let me know, if you'd like me to send the updated webrev set.

With kind regards,
Ivan


On 3/27/18 6:47 AM, Chris Hegarty wrote:

Ivan,


On 26 Mar 2018, at 23:08, Ivan Gerasimov  wrote:

Hello everyone!

A few modifications were done to the patch.
Below is the list of updated parts of the patch with comments about what has 
changed, comparing to the previous version of the patch.

The patched JDK builds fine and all the tests pass well.

WEBREV-000: http://cr.openjdk.java.net/~igerasim/8198358/04/000/webrev/

Ok.


WEBREV-001: http://cr.openjdk.java.net/~igerasim/8198358/04/001/webrev/

Ok.


WEBREV-002: http://cr.openjdk.java.net/~igerasim/8198358/04/002/webrev/

Can we make fdAccess final ( and in DualStack too )
 static *final* JavaIOFileDescriptorAccess fdAccess

JNIEXPORT void JNICALL Java_java_net_TwoStacksPlainSocketImpl_bind0 ...

  142 if (IS_NULL(iaObj)) {
  143 JNU_ThrowNullPointerException(env, "inet address argument");
  144 return;
  145 }
  146
  147 family = getInetAddress_family(env, iaObj);
  148 if (family != java_net_InetAddress_IPv4) {
  149 JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException",
  150 "Protocol family not supported");
  151 return;
  152 }

The address is already null checked at the java level, so can be removed here, 
no?

Can the family check be hoisted up into Java? I think this will help when it 
comes
to the eventual merge with DualStack.

The native code will then align with DualStack’s.  I prefer to reduce and align
the native layers as much as possible. It’s easier to deal with differences at
the Java level.


WEBREV-003: http://cr.openjdk.java.net/~igerasim/8198358/04/003/webrev/
- Rebased to reflect recent changes to socketListen()

Ok.


WEBREV-004: http://cr.openjdk.java.net/~igerasim/8198358/04/004/webrev/

Ok.


WEBREV-005: http://cr.openjdk.java.net/~igerasim/8198358/04/005/webrev/

Ok.


WEBREV-006: http://cr.openjdk.java.net/~igerasim/8198358/04/006/webrev/

Ok.


WEBREV-007: http://cr.openjdk.java.net/~igerasim/8198358/04/007/webrev/

Ok.


WEBREV-008: http://cr.openjdk.java.net/~igerasim/8198358/04/008/webrev/
- The check the line 400 was changed to test if (newfd < 0) and then if (newfd 
== -2).
I don't think the later is quite accurate (cannot find in the documentation 
that accept can return -2), but it is how it was done in TwoStacks originally, 
so no regression is expected.

Ok.

  413 if (sa.sa.sa_family != AF_INET) {
  414 JNU_ThrowByName(env, JNU_JAVANETPKG "SocketException",
  415 "Protocol family not supported");
  416 NET_SocketClose(newfd);
  417 return -1;
  418 }

This check could be hoisted up to the Java-level too, no? Again, I think it
would be easier to deal with this kinda differences in Java.


WEBREV-009: http://cr.openjdk.java.net/~igerasim/8198358/04/009/webrev/
- Added missing 'return -1;' at the line 188.

Can the family be checked at the java level.


WEBREV-010: http://cr.openjdk.java.net/~igerasim/8198358/04/010/webrev/

Ok.


WEBREV-011: http://cr.openjdk.java.net/~igerasim/8198358/04/011/webrev/

Ok.


WEBREV-012: http://cr.openjdk.java.net/~igerasim/8198358/04/012/webrev/

Ok.


WEBREV-013: http://cr.openjdk.java.net/~igerasim/8198358/04/013/webrev/

Ok.


WEBREV-014: http://cr.openjdk.java.net/~igerasim/8198358/04/014/webrev/

Good.

-Chris.




--
With kind regards,
Ivan Gerasimov



Re: RFR [11] 8198358 : Align organization of DualStackPlainSocketImpl with TwoStacksPlainSocketImp [win]

2018-03-27 Thread Chris Hegarty

> On 27 Mar 2018, at 18:30, Ivan Gerasimov  wrote:
> 
> Thank you Christoph and Chris for the review!
> 
> I made the following suggested changes:
> - fdAccess made private static final in both TwoStacks and DualStack.
> - removed redundant check IS_NULL(iaObj) at bind0 (indeed, the address was 
> already checked at Java level).
> 
> For now, I would prefer to keep the checks for family == IPv4 as they are.
> I totally agree they should be moved to Java, but I'm planning to do this 
> during the TwoStacks-DualStack merge step.
> 
> I'm also planning to add a regtest to make sure that Inet6Address is rejected 
> when fed to a IPv4-only socked.
> 
> Are we good to push now?

Yes.

-Chris.



Re: RFR [11] 8197564: HTTP Client implementation - JEP 321

2018-03-27 Thread James Roper
Hi Chris,

Requiring a third party adapter for using Reactive Streams for WebSockets
is not the end of the world, so I don't want to push this too hard, we can
just agree to disagree. But there were a few small questions in your reply
that I'll answer.

> Will their backpressure mechanisms map to each other easily?
>
> I see no reason that it should not. Are you aware of any?
>

It would take actually implementing an adapter to be fully aware. Note that
the Reactive Streams request() mechanism is very strictly specified,
particularly around the edge cases. As an example of just one case, what
happens if you invoke:

request(16);
request(Long.MAX_VALUE);

This is explicitly allowed by the reactive streams spec, and the expected
behavior is as as soon as demand reaches Long.MAX_VALUE, the demand is
considered to be infinite. This is also an implementation detail that many
implementations get wrong, but that's ok because the Reactive Streams TCK
verifies this behavior to ensure they get it right. But it's unspecified in
the HTTP client spec (or in the javadocs at least), so will it map nicely?
I don't know. Also, the above scenario is very possible - for example, an
implementation might consume a certain number of messages, and then decide
that it wants to ignore the rest (ignore, as opposed to push back and
block, or cancel which would close the socket), and so they just request
Long.MAX_VALUE to indicate that.

>  Will their failure handling and error handling semantics map to each
> other easily?
>
> I see no reason that they should not. In fact, most higher-level APIs
> provide more coarse grain error handling.
>

So for a start, Reactive Streams prohibits any method from throwing an
exception - for example, if you invoke request(-1), you're not allowed to
throw an IllegalArgumentException, you have to instead pass the exception
to onError.

But a lot of other methods in the WebSocket spec are allowed to throw
IOException. How a Reactive Streams implementation handles these is
unspecified, so in order to safely ensure that errors are propagated (and
the important thing here is to make sure that the right errors appear in
the right place at the right time), an adapter will have to wrap all these
methods, and implement the necessary handling to pass the errors to
onError, invoke the upstream cancel, and then ensure that after that, no
further callbacks are delegated through in either direction, and given that
this is a concurrent API with callbacks being invoked by different threads
in both directions, that's going to require things like
AtomicReference.compareAndSet, etc. Doing it correctly, ensuring there are
no race conditions, ensuring that and end developer can 100% predictably
know that all errors will be handled in a predictable way, is not at all
trivial.

-- 
*James Roper*
*Senior Octonaut*

Lightbend  – Build reactive apps!
Twitter: @jroper 


Re: RFR [11] 8197564: HTTP Client implementation - JEP 321

2018-03-27 Thread James Roper
On 23 March 2018 at 04:36, Wenbo Zhu  wrote:
>
> +1
>
> Message-based flow-control is unspecified by the websocket protocol; and
> exposing byte-based flow-control (via RS) as part of the WS API seems a
> rather questionable approach.
>

No one is suggesting a byte based flow control mechanism, RS flow control
is never byte based (unless you use Publisher/Subscriber, but
no one ever does that because it's incredibly inefficient). Not even TCP
supports byte based flow control, it's packet based. I'm not sure why
you're raising byte based flow control here.

The WebSocket protocol does have implied flow control semantics by virtue
of the fact that it is layered on top of TCP. The current WebSocket API in
JEP 321 offers frame based flow control using an API very similar to RS,
which for all intents and purposes, is message based flow control, since
I've never actually seen, in the wild, fragmented WebSocket messages be
used. Of course, due to the mismatch of TCP packets, OS buffers and
WebSocket frames, there is lag in the propagation of flow control while
buffers fill up, but it is none the less there.

Any useful RS support will have to involve some application-level protocols.
>

I completely disagree with this statement. As someone who has been involved
in/led the implementation of multiple WebSocket clients and servers
(including Play Framework, Lagom Framework and Akka HTTP), I can tell you
from my experience that after the initial WebSocket handshake, WebSockets
can be offered to application developers as just streams of messages using
Reactive Streams, without any additional WebSocket protocol exposure, and
still be useful for 99% of use cases. It certainly does not have to involve
exposing the protocol to the application, most applications care nothing
about message fragmentation, they care nothing about close codes or reasons
beyond whether the connection was terminated with a success or error, and
they care nothing about pings/pongs. Everything that they do care about,
Reactive Streams gives a way of expressing.

> with the understanding that such a helper will limit the capabilities and
>> performance when its used - eg, the range of options for closing, range of
>> options for how errors are handled, and perhaps introduce some small
>> performance penalties such as additional allocations and/or buffer copies.
>>
>> That is a whole set of comprises and decisions that would need to
>> be considered.
>>
>> > The use case here is how well this API will interact with other APIs.
>> WebSockets are rarely used in isolation from other technologies. Most non
>> trivial usages of WebSockets will involve plumbing the WebSocket API to
>> some other source or sink of streaming data - for example, a message
>> broker, a database, perhaps a gRPC stream to another service.
>>
>> I could well image a set of vertical-market specific libraries being built
>> on top of the WebSocket API, and that is how it should be. In this
>> specific area, providing the lowest level API is an enabler for others.
>>
>> > The question is, how difficult will it be to connect those two APIs?
>>
>> I imagine some work may be required, but certainly much less than
>> without the WebSocket API.
>>
>> > Will their backpressure mechanisms map to each other easily?
>>
>> I see no reason that it should not. Are you aware of any?
>>
>> >  Will their failure handling and error handling semantics map to each
>> other easily?
>>
>> I see no reason that they should not. In fact, most higher-level APIs
>> provide more coarse grain error handling.
>>
>> > How many lines of boilerplate will it take to hook them up?
>>
>> I guess that depends on the particular library's functionality, will it
>> support/handle/expose partial message delivery, buffering, etc. I
>> don’t see this as boilerplate though.
>>
>> > If the WebSocket provides an option to use Reactive Streams, then for
>> any other sources/sink of streamed data that support Reactive Streams,
>> application developers will have an option where the answers are guaranteed
>> to be yes without the developer having to do any mapping themselves, and
>> the lines of code to do that will be one.
>>
>> Sure, if one has a higher-level library that works with Reactive Streams,
>> and one wants to use WebSocket as a transport, then it would be little
>> work if Java SE provided a Reactive Streams interface to WebSocket.
>> There are also vertical-markets using WebSocket, but not using Reactive
>> Streams.
>>
>> At this point in JEP 321, I do not want to increase the scope of the
>> project
>> to include a Reactive Streams interface for WebSocket. This is something
>> that can, and should, be considered separate to JEP 321. It could follow
>> in a future release if deemed appropriate, or not, but adding it at this
>> point
>> will add risk to JEP 321, which I am not willing to do.
>>
>> -Chris.
>>
>>
>


-- 
*James Roper*
*Senior Octonaut*

Lightbend  – Build reactive apps!
Tw