RFR 8163083: SocketListeningConnector does not allow invocations with port 0

[Daniil Titov]

HI JC,

 

The webrev is updated to address this. 

 

Webrev: http://cr.openjdk.java.net/~dtitov/8163083/webrev.06

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

Thanks!
--Daniil

 

 

From: JC Beyler <jcbeyler at google.com>
Date: Monday, September 24, 2018 at 8:47 PM
To: <daniil.x.titov at oracle.com>
Cc: <alexey.menkov at oracle.com>, <gary.adams at oracle.com>, <serviceability-dev at openjdk.java.net>
Subject: Re: RFR 8163083: SocketListeningConnector does not allow invocations with port 0

 

Hi Daniil,

 

Still looks good to me :)

 

Nit: empty line 83 of the new test is not required!

Jc

 

On Mon, Sep 24, 2018 at 5:54 PM Daniil Titov <daniil.x.titov at oracle.com> wrote:

Hi Alex,

Please review the updated webrev that has new test moved to test/jdk/comsun/jdi/connect folder.

Webrev: http://cr.openjdk.java.net/~dtitov/8163083/webrev.05/
Bug: https://bugs.openjdk.java.net/browse/JDK-8163083

Thanks!
--Daniil


On 9/24/18, 2:56 PM, "Alex Menkov" <alexey.menkov at oracle.com> wrote:

    Daniil,

    Just remembered that SQE requested to not add new tests to vmTestbase 
    (see test/hotspot/jtreg/vmTestbase/README.md)
    Could you please move the test to correct location (I suppose it's 
    test/jdk/com/sun/jdi)

    --alex


    On 09/24/2018 14:30, Alex Menkov wrote:
    > +1
    > 
    > --alex
    > 
    > On 09/24/2018 10:39, Gary Adams wrote:
    >> Looks good to me.
    >>
    >> On 9/24/18, 1:25 PM, Daniil Titov wrote:
    >>> Hi Alex and Gary,
    >>>
    >>> Please review the updated patch that includes suggested changes.
    >>>
    >>> http://cr.openjdk.java.net/~dtitov/8163083/webrev.04/
    >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8163083
    >>>
    >>> Thanks!
    >>> --Daniil
    >>>
    >>>
    >>>
    >>>
    >>> On 9/21/18, 3:59 PM, "Alex Menkov"<alexey.menkov at oracle.com>  wrote:
    >>>
    >>>      One more note:
    >>>      please add "@Override" annotation to the
    >>>      SocketListeningConnector.updateArgumentMapIfRequired
    >>>
    >>>      --alex
    >>>
    >>>      On 09/21/2018 02:26, gary.adams at oracle.com wrote:
    >>>      >  Looks good to me.
    >>>      >
    >>>      >  For the javadoc
    >>>      >
    >>>      >     72      *<p>
    >>>      >     73      * If<code>arguments</code>  contains addressing 
    >>> information. and
    >>>      >     74      * only one connection will be accepted, the {@link 
    >>> #accept accept} method
    >>>      >     75      * can be called immediately without calling this 
    >>> method.
    >>>      >     76      *
    >>>      >  77 * If the addressing information provided 
    >>> in<code>arguments</code>
    >>>      >  implies
    >>>      >  78 * the auto detection this information might be updated 
    >>> with the address
    >>>      >  79 * of the started listener.
    >>>      >
    >>>      >     - you could add a<p>  tag if you want to maintain the 
    >>> spacing in the
    >>>      >  generated javadoc.
    >>>      >     - I've seen other changesets migrating to {@code ..} from 
    >>> the older
    >>>      >  <code>...</code>
    >>>      >
    >>>      >  It would be good to include some negative testing.
    >>>      >  Not sure if it is already covered in other tests, e.g.
    >>>      >
    >>>      >       args1 = defaultArguments()
    >>>      >       startListening(args1)   // bound port updated
    >>>      >       startListening(args1)   // already listening
    >>>      >
    >>>      >
    >>>      >  On 9/20/18 10:59 PM, Daniil Titov wrote:
    >>>      >>  Please review the change that fixes the issue in 
    >>> com.sun.tools.jdi.SocketListenerConnector.startListening() method.
    >>>      >>
    >>>      >>  When the argument map passed to startListening() methods has 
    >>> the port number unspecified or set to zero the port is auto detected. 
    >>> However,  the consequent call of startListening() methods with 
    >>> unspecified port number fails rather than starts a new listener on 
    >>> auto detected port. This happens since the original argument map 
    >>> passed to the startListening() methods is used as a key to store the 
    >>> mapping to the started listeners.
    >>>      >>
    >>>      >>  The fix ensures that in cases when the port is auto detected 
    >>> the argument map is updated with the bound port number.
    >>>      >>
    >>>      >>  Mach5 vmTestbase_nsk_jdi tests successfully passed.
    >>>      >>
    >>>      >>  Bug:https://bugs.openjdk.java.net/browse/JDK-8163083
    >>>      >>  Webrev:http://cr.openjdk.java.net/~dtitov/8163083/webrev.02/
    >>>      >>
    >>>      >>  Thanks!
    >>>      >>  --Daniil
    >>>      >>
    >>>      >>
    >>>      >>
    >>>      >
    >>>
    >>>
    >>>
    >>




 

-- 

 

Thanks,

Jc

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180925/751a53d6/attachment.html>