RFR 8163083: SocketListeningConnector does not allow invocations with port 0

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


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>
> *Organization: *Oracle Corporation
> *Date: *Wednesday, September 26, 2018 at 8:12 PM
> *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
>
> 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/21dbe36f/attachment-0001.html>


More information about the serviceability-dev mailing list