RFR 8163083: SocketListeningConnector does not allow invocations with port 0
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Thu Sep 27 03:12:39 UTC 2018
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>
> *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 <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/20180926/fa0428e7/attachment-0001.html>
More information about the serviceability-dev
mailing list