RFR 8163083: SocketListeningConnector does not allow invocations with port 0

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Thu Sep 27 18:58:07 UTC 2018


It looks good to me.

Thanks!
Serguei

On 9/27/18 11:50 AM, Daniil Titov wrote:
>
> Hi Serguei,
>
> The webrev was updated to address all these comments.
>
> Webrev: http://cr.openjdk.java.net/~dtitov/8163083/webrev.08 
> <http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.08>
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8163083
>
> Thanks!
>
> --Daniil
>
> *From: *<serguei.spitsyn at oracle.com>
> *Organization: *Oracle Corporation
> *Date: *Thursday, September 27, 2018 at 11:33 AM
> *To: *Daniil Titov <daniil.x.titov at oracle.com>, JC Beyler 
> <jcbeyler at google.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
>
> Just noticed one more minor issue:
>
> +import java.util.Collection;
> +import java.util.Iterator;
> +import java.util.LinkedHashMap;
>
>
>  It seems the above imports are not really used and can be removed.
>
> Thanks,
> Serguei
>
> On 9/27/18 11:22 AM, serguei.spitsyn at oracle.com 
> <mailto:serguei.spitsyn at oracle.com> wrote:
>
>     Hi Daniil,
>
>     It looks great, thank you for the update.
>     I have a couple of more minor comments on the test.
>
>        56         testWithDefaultArgs(connector);
>
>        57         testWithDefaultArgs2(connector);
>
>        58         testWithWildcardPort1(connector);
>
>        59         testWithWildcardPort2(connector);
>
>
>       Please, rename testWithDefaultArgs to testWithDefaultArgs1
>       to make naming consistent.
>
>       111             throw new RuntimeException("Test testWithDefaultArgsNegative failed. The argument map was not updated with" +
>
>       112                     " the bound port number.");
>
>       115             // This call should fail since the previous the argument map is already updated with the port
>
>       116             // number of the started listener
>
>
>       Could you, please, re-balance the above line pairs to make the
>     first line shorter?
>
>     No need in new webrev if you fix the above.
>
>     Thanks,
>     Serguei
>
>     On 9/27/18 11:00 AM, Daniil Titov wrote:
>
>         Hi Serguei,
>
>         Thank you for reviewing this change. Initially I understood
>         the whole fragment below  (from the Javadoc for
>         com.sun.jdi.connect.ListeningConnector.startListening()
>         method) as a direction for the user how to obtain and prepare
>         the argument map before passing it in startListening() method.
>
>            61      * The argument map associates argument name strings to instances
>
>            62      * of {@link Connector.Argument}. The default argument map for a
>
>            63      * connector can be obtained through {@link Connector#defaultArguments}.
>
>            64      * Argument map values can be changed, but map entries should not be
>
>            65      * added or deleted.
>
>         But I agree that the last sentence could also mean that the
>         argument map values could be changes as a result of the method
>         invocation and in this case the new fragment in the Javadoc is
>         not needed.
>
>         Please review the updated webrev that does not add new Javadoc
>         fragment and includes other changes you suggested.
>
>
>         Webrev: http://cr.openjdk.java.net/~dtitov/8163083/webrev.07/
>         <http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.07/>
>
>         Bug: https://bugs.openjdk.java.net/browse/JDK-8163083
>
>         Thanks!
>
>         --Daniil
>
>         *From: *<serguei.spitsyn at oracle.com>
>         <mailto:serguei.spitsyn at oracle.com>
>         *Organization: *Oracle Corporation
>         *Date: *Wednesday, September 26, 2018 at 8:12 PM
>         *To: *Daniil Titov <daniil.x.titov at oracle.com>
>         <mailto:daniil.x.titov at oracle.com>, JC Beyler
>         <jcbeyler at google.com> <mailto:jcbeyler at google.com>
>         *Cc: *<alexey.menkov at oracle.com>
>         <mailto:alexey.menkov at oracle.com>, <gary.adams at oracle.com>
>         <mailto:gary.adams at oracle.com>,
>         <serviceability-dev at openjdk.java.net>
>         <mailto:serviceability-dev at openjdk.java.net>
>         *Subject: *Re: RFR 8163083: SocketListeningConnector does not
>         allow invocations with port 0
>
>         Hi Daniil,
>
>         It is great that you came up the fix for this issue.
>         Thanks to Gary for the help too.
>
>         I wonder if we could get away without updating the javadoc
>         of com/sun/jdi/connect/ListeningConnector.java.
>         Filing a CSR would not be needed then (simple javadoc
>         reformatting should not require a CSR).
>
>         So, my question is if this new fragment is really needed:
>
>           76      * <p>
>
>           77      * If the addressing information provided in {@code
>         arguments} implies
>
>           78      * the auto detection this information might be
>         updated with the address
>
>           79      * of the started listener.
>
>         The javadoc for startListening already has this fragment:
>
>            61      * The argument map associates argument name strings to instances
>
>            62      * of {@link Connector.Argument}. The default argument map for a
>
>            63      * connector can be obtained through {@link Connector#defaultArguments}.
>
>            64      * Argument map values can be changed, but map entries should not be
>
>            65      * added or deleted.
>
>           
>
>         that allows to change the argument map values.
>
>         It looks like, it has to be Okay to add the bound port number there.
>
>           
>
>         Please, let me know what you think.
>
>         Some formatting comments to addition to Jc's comments:
>
>           77      * If the addressing information provided in {@code
>         arguments} implies
>
>           78      * the auto detection this information might be
>         updated with the address
>
>           79      * of the started listener.
>
>           
>
>            This sentence needs to be split in two:
>
>           
>
>           77      * If the addressing information provided in {@code
>         arguments} implies
>
>           78      * the auto detection. This information might be
>         updated with the address
>
>           79      * of the started listener.
>
>         +
>
>         +    protected void updateArgumentMapIfRequired(
>
>         +            Map<String, ? extends Connector.Argument>
>         args,TransportService.ListenKey listener) {
>
>         +    }
>
>         +
>
>           The indent has to be 4, not 8.
>
>         +        if(isWildcardPort(args)) {
>
>         +            String[] address = listener.address().split(":");
>
>         +            if (address.length > 1) {
>
>         +                args.get(ARG_PORT).setValue(address[1]);
>
>         +            }
>
>         +        }
>
>           A space is missed after the first 'if'.
>
>            50                         filter(c ->
>
>            51                                 c.name().equals("com.sun.jdi.SocketListen")).findFirst().get();
>
>           This is better to be one liner.
>
>            57         testWithDefaultArgs(connector);
>
>            58         testWithDefaultArgs2(connector);
>
>            59         testWithWildcardPort(connector);
>
>            60         testWithWildcardPort2(connector);
>
>           
>
>            A suggestion is to rename above as below to have the names more unified:
>
>           
>
>            57         testWithDefaultArgs1(connector);
>
>            58         testWithDefaultArgs2(connector);
>
>            59         testWithWildcardPort1(connector);
>
>            60         testWithWildcardPort2(connector);
>
>           
>
>         Thanks,
>         Serguei
>
>
>         On 9/25/18 10:32 AM, Daniil Titov wrote:
>
>             HI JC,
>
>             The webrev is updated to address this.
>
>             Webrev:
>             http://cr.openjdk.java.net/~dtitov/8163083/webrev.06
>             <http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.06>
>
>             Bug: https://bugs.openjdk.java.net/browse/JDK-8163083
>
>             Thanks!
>             --Daniil
>
>             *From: *JC Beyler <jcbeyler at google.com>
>             <mailto:jcbeyler at google.com>
>             *Date: *Monday, September 24, 2018 at 8:47 PM
>             *To: *<daniil.x.titov at oracle.com>
>             <mailto:daniil.x.titov at oracle.com>
>             *Cc: *<alexey.menkov at oracle.com>
>             <mailto:alexey.menkov at oracle.com>, <gary.adams at oracle.com>
>             <mailto:gary.adams at oracle.com>,
>             <serviceability-dev at openjdk.java.net>
>             <mailto: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
>             <mailto: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/
>                 <http://cr.openjdk.java.net/%7Edtitov/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
>                 <mailto: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/
>                 <http://cr.openjdk.java.net/%7Edtitov/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
>                 <mailto: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 <mailto: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/
>                 <http://cr.openjdk.java.net/%7Edtitov/8163083/webrev.02/>
>                     >>>      >>
>                     >>>      >>  Thanks!
>                     >>>      >>  --Daniil
>                     >>>      >>
>                     >>>      >>
>                     >>>      >>
>                     >>>      >
>                     >>>
>                     >>>
>                     >>>
>                     >>
>
>
>
>
>
>             -- 
>
>             Thanks,
>
>             Jc
>
>
>
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180927/68f3751e/attachment-0001.html>


More information about the serviceability-dev mailing list