RFR 8163083: SocketListeningConnector does not allow invocations with port 0
Daniil Titov
daniil.x.titov at oracle.com
Thu Sep 27 18:50:01 UTC 2018
Hi Serguei,
The webrev was updated to address all these comments.
Webrev: http://cr.openjdk.java.net/~dtitov/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 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/
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
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> 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/
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> 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/
>>> 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> wrote:
>>>
>>> One more note:
>>> please add "@Override" annotation to the
>>> SocketListeningConnector.updateArgumentMapIfRequired
>>>
>>> --alex
>>>
>>> On 09/21/2018 02:26, 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/
>>> >>
>>> >> Thanks!
>>> >> --Daniil
>>> >>
>>> >>
>>> >>
>>> >
>>>
>>>
>>>
>>
--
Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20180927/eed04939/attachment-0001.html>
More information about the serviceability-dev
mailing list