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