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