8232673: (dc) DatagramChannel socket adaptor issues

[Daniel Fuchs]

Hi Alan,

Thanks for this patch that fixes and brings into the mainline
many of the small issues we noticed while working on JDK-8230211.

I looked at the patch and I think it is good.

A couple of comments still:

DatagramChannelImpl:

  452                     assert sender != null;

pedantically that should be:

                          assert sender != null || !isOpen();

Indeed if the channel is closed at this point the finally clause
at lines:

  456                 endRead(blocking, n > 0);
  457                 assert IOStatus.check(n);

will either erase the assertion error by replacing it with an
IOException thrown by endRead() at line 456 or will replace
it by another assertion error at line 457. In any case it should be
legal for `sender` to be null at line 452 if the channel has
been asynchronously closed or closed by interrupt.

The same would apply for asserts at lines:
  566             assert sender != null;
  601                 assert sender != null;


I also believe a similar-ish (but maybe more relevant) kind of
issue would arise for:

  491                 assert n >= 0;

at this point - and as per line

  484                     while (IOStatus.okayToRetry(n) && isOpen()) {

then n could well be < 0 if the channel is closed.
I suspect the assertion at line 491 should therefore be preceded
with:

    if (n < 0) return n;

either after line 487 or after line 490.

best regards,

-- daniel


On 27/10/2019 14:16, Alan Bateman wrote:
> DatagramChannel::socket returns a socket adaptor, essentially a 
> DatagramSocket view on the channel. The original motivation for the 
> socket method was binding and socket options but it also created the 
> potential for migration or interop with code using the older 
> DatagramSocket API. The adaptor has been on my radar for sometime as it 
> implements a blocking receive method that relies on the channel's 
> blockingLock. This locking is problematic for several reasons, including 
> preventing a send to proceed when a thread is blocked in receive. The 
> main change in the patch proposed here is to re-implememt the adaptor's 
> send and receive methods so they can execute concurrently and don't 
> block while holding a built-in monitor. I've also changed these methods 
> to use the buffer cache mechanism and avoid creating a ByteBuffer per 
> I/O operation. The new receive implementation also works around two 
> flaws in the legacy DatagramPacket API/implementation (discussed in 
> JDK-8232817).
> 
> While in the area, a few other smaller issues are fixed too:
> 
> 1. A connected DatagramChannel needs to disconnect before connecting it 
> to another address. DatagramSocket doesn't require this. The adaptor is 
> changed to allow it connect without throwing an exception. The connect 
> methods are also fixed to throw IllegalArgumentException rather than 
> NullPointerException when invoked with a null address.
> 
> 2. The adaptor's getLocalSocketAddress method returns the local address 
> when channel was closed. This is an inconsistency in DatagramSocket's 
> API that was missed in the adaptor.
> 
> I've expanded the test coverage. The new AdaptorGetters test attempts to 
> exercise all the getter methods when the channel is in different states 
> so gives us confidence that there aren't further issues with these methods.
> 
> The webrev with the proposed changes is here:
>      http://cr.openjdk.java.net/~alanb/8232673/webrev/index.html
> 
> -Alan