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