8222774: (ch) Replace uses of stateLock and blockingLock with explicit locks

[Daniel Fuchs]

Hi Alan,

I have read through the patch and didn't see anything
too surprising. I might not have followed all the subtleties
though - so another pair of eyes would be good.

in DatagramChannelImpl.java, a few places use:

  458                         if (blocking) {
  459                             while (IOStatus.okayToRetry(n) && 
isOpen()) { ... }

and a few others use

  549                     if (blocking && IOStatus.okayToRetry(n)) {
  550                         do { ...
  553                         } while (IOStatus.okayToRetry(n) && isOpen());

I was wondering about the asymmetry. It looks like
the former if (blocking) { while() { ... } } might be more
correct, as it ensure that isOpen() is also checked between
the first and second call to the IO primitive which is
being wrapped? (same goes for ServerSocketChannelImpl.java
SocketChannelImpl.java, SinkChannelImpl.java)

DatagramSocketAdaptor.java:

  279     @Override
  280     public int getSoTimeout() throws SocketException {
  281         if (!dc.isOpen())
  282             throw new SocketException("Socket is closed");

Is there any backward compatibility concern with this
change? I mean - I understand that setting a value on a
closed socket makes no sense. But could some code out
there be trying to figure out what was set on the
socket before it was closed, e.g. for diagnosis purposes?
(same goes for ServerSocketAdaptor.java)


AdaptorStreams.java:

Has this test run through tier8? This is where the special
VM configurations happen - and it's been known to trigger
intermittent failure more often than the other tiers.

I see the test use 1000 and 2000 millis, reasoning that if
the timeout is 2000ms then something scheduled to happen
after 1000ms should not trigger the timeout - and I wonder
how stable that is going to be.

I see the test uses ServerSocket(0). We manage to eliminate
most of the intermittent failures in the new HttpClient tests
by consistently binding out server sockets to the loopback
address, and avoiding the wildcard. I wonder if we should
do it here too.

best regards,

-- daniel


On 22/04/2019 12:08, Alan Bateman wrote:
> 
> In JDK 11 we changed most of the locking in the network channels to use 
> explicit locks in preparation for a future world of user-mode threads 
> [1]. I'd like to do another round of update in this area to change the 
> stateLock to an explicit lock and also re-visit the implementation of 
> the socket adaptors that currently make use of the channel blockingLock.
> 
> The motivation for changing stateLock is that closing a channel is a 
> blocking operation when it must coordinate the close with threads that 
> are blocked in I/O operations on the channel. Changing this to an 
> explicit lock it a simple translation although it touches a lot of 
> places. As part of this the closeLock used in 
> AbstractInterruptibleChannel also needs to be changed.
> 
> The changes to support the socket adaptors is a bit trickery. Timed 
> connect/accept/read (via the adaptors) require the underlying socket to 
> be temporarily changed to non-blocking mode whilst the channel remains 
> in blocking mode. In the case of timed read then this should be possible 
> while another thread is doing a blocking write - that blocking write 
> will happen while the underlying socket is in non-blocking mode. To keep 
> it simple, and avoid another round of changes later, all the blocking 
> operations are changed so that they can work even if the underlying 
> socket is non-blocking.
> 
> While re-visiting the socket adaptors I spotted several issues/bugs that 
> have not been reported. I'll create issues to follow-up on those. 
> However in passing, there are a few small changes to the socket adaptor, 
> one of which is to create each socket adaptos with a dummy SocketImpl. 
> Michael McMahon is working on JDK-8216978 [2] to drop support for pre 
> JDK 1.4 SocketImpls - if that change gets rid the annoying references 
> from the SocketImpl to the enclosing Socket/ServerSocket then it will 
> allow all socket adaptors to share a dummy SocketImpl.
> 
> The existing tests provide reasonable coverage of the changes. The only 
> area that wasn't tested (as it never really worked) is concurrent use of 
> the input and output streams obtained from a socket adaptor. The new 
> test attempts to exercise all the possible scenarios.
> 
> There are two items that aren't addressed in this webrev:
> 
> 1. DatagramSocket's socket adaptor. The send/receive methods defined by 
> legacy DatagramSocket synchronizes on the datagram packet. It requires 
> further investigation although I suspect it may not be feasible to do 
> anything here for compatibility reasons.
> 
> 2. Interrupting a thread blocked in I/O operation will close the channel 
> so that makes Thread.interrupt a blocking operation. The changes for 
> this to play well with user mode threads touches other areas so I'll 
> separate that out for now.
> 
> The webrev with the proposed changes is here:
> http://cr.openjdk.java.net/~alanb/8222774/0/webrev/index.html
> 
> -Alan
> 
> [1] 
> https://mail.openjdk.java.net/pipermail/nio-dev/2018-February/004683.html
> [2] https://bugs.openjdk.java.net/browse/JDK-8216978