Discussion:
RFR JDK-8212151: jdi/ExclusiveBind.java times out due to "bind
Alex Menkov
2018-10-19 16:59:24 UTC
Permalink
Hi all,

jira: https://bugs.openjdk.java.net/browse/JDK-8212151
webrev: http://cr.openjdk.java.net/~amenkov/exclusiveBind/webrev/

The fix updates the test to allow debuggee to select available port
instead of using error-prone "getFreePort" approach.

--alex
JC Beyler
2018-10-19 17:34:33 UTC
Permalink
Hi Alex,

I remember seeing this same code so went looking for it and saw it in
JdbTest.java (you added it here it seems:
http://hg.openjdk.java.net/jdk/jdk/rev/083e731da31a).

I have two few questions:
- Does it make sense to put this code in a helper method?
- The code you added in JdbTest.java does not do the adjusted time for 30
it seems, is that normal?

Thanks,
Jc
Post by Alex Menkov
Hi all,
jira: https://bugs.openjdk.java.net/browse/JDK-8212151
webrev: http://cr.openjdk.java.net/~amenkov/exclusiveBind/webrev/
The fix updates the test to allow debuggee to select available port
instead of using error-prone "getFreePort" approach.
--alex
--
Thanks,
Jc
Alex Menkov
2018-10-19 19:36:45 UTC
Permalink
Hi Jc,
Post by JC Beyler
Hi Alex,
I remember seeing this same code so went looking for it and saw it in
http://hg.openjdk.java.net/jdk/jdk/rev/083e731da31a).
  - Does it make sense to put this code in a helper method?
I tried to develop some shared code, but it didn't look good (to me :)
I'll rethink this one more time.
Post by JC Beyler
  - The code you added in JdbTest.java does not do the adjusted time
for 30 it seems, is that normal?
Actually adjustment is not needed here as ProcessTools.startProcess
adjust timeout internally.

--alex
Post by JC Beyler
Thanks,
Jc
Hi all,
jira: https://bugs.openjdk.java.net/browse/JDK-8212151
webrev: http://cr.openjdk.java.net/~amenkov/exclusiveBind/webrev/
<http://cr.openjdk.java.net/%7Eamenkov/exclusiveBind/webrev/>
The fix updates the test to allow debuggee to select available port
instead of using error-prone "getFreePort" approach.
--alex
--
Thanks,
Jc
Alex Menkov
2018-10-19 22:56:19 UTC
Permalink
Hi Jc,

Updated fix:
http://cr.openjdk.java.net/~amenkov/exclusiveBind/webrev.01/
Moved shared code to new Debuggee class.

--alex
Post by JC Beyler
Hi Alex,
I remember seeing this same code so went looking for it and saw it in
http://hg.openjdk.java.net/jdk/jdk/rev/083e731da31a).
  - Does it make sense to put this code in a helper method?
  - The code you added in JdbTest.java does not do the adjusted time
for 30 it seems, is that normal?
Thanks,
Jc
Hi all,
jira: https://bugs.openjdk.java.net/browse/JDK-8212151
webrev: http://cr.openjdk.java.net/~amenkov/exclusiveBind/webrev/
<http://cr.openjdk.java.net/%7Eamenkov/exclusiveBind/webrev/>
The fix updates the test to allow debuggee to select available port
instead of using error-prone "getFreePort" approach.
--alex
--
Thanks,
Jc
JC Beyler
2018-10-21 03:29:07 UTC
Permalink
Hi Alex,

It looks really good to me now. The test and JdbTest are now easier to read
because we do not have the noise of the debuggee trying to attach.

A few nits:
- Small nit really is the fact that the javadoc for Debuggee states two
usages and JdbTest does not use either :)
- You put a few get methods in Debuggee that are seemingly not used
anywhere, we could add them later if need be and not have them now?

Thanks for this, I think it's really better in the long run to have this in
one centralized spot,
Jc
Post by Alex Menkov
Hi Jc,
http://cr.openjdk.java.net/~amenkov/exclusiveBind/webrev.01/
Moved shared code to new Debuggee class.
--alex
Post by JC Beyler
Hi Alex,
I remember seeing this same code so went looking for it and saw it in
http://hg.openjdk.java.net/jdk/jdk/rev/083e731da31a).
- Does it make sense to put this code in a helper method?
- The code you added in JdbTest.java does not do the adjusted time
for 30 it seems, is that normal?
Thanks,
Jc
Hi all,
jira: https://bugs.openjdk.java.net/browse/JDK-8212151
webrev: http://cr.openjdk.java.net/~amenkov/exclusiveBind/webrev/
<http://cr.openjdk.java.net/%7Eamenkov/exclusiveBind/webrev/>
The fix updates the test to allow debuggee to select available port
instead of using error-prone "getFreePort" approach.
--alex
--
Thanks,
Jc
--
Thanks,
Jc
Alex Menkov
2018-10-23 00:24:38 UTC
Permalink
Hi Jc,

new webrev:
http://cr.openjdk.java.net/~amenkov/exclusiveBind/webrev.02/

--alex
Post by JC Beyler
Hi Alex,
It looks really good to me now. The test and JdbTest are now easier to
read because we do not have the noise of the debuggee trying to attach.
  - Small nit really is the fact that the javadoc for Debuggee states
two usages and JdbTest does not use either :)
  - You put a few get methods in Debuggee that are seemingly not used
anywhere, we could add them later if need be and not have them now?
Thanks for this, I think it's really better in the long run to have this
in one centralized spot,
Jc
Hi Jc,
http://cr.openjdk.java.net/~amenkov/exclusiveBind/webrev.01/
<http://cr.openjdk.java.net/%7Eamenkov/exclusiveBind/webrev.01/>
Moved shared code to new Debuggee class.
--alex
Post by JC Beyler
Hi Alex,
I remember seeing this same code so went looking for it and saw
it in
Post by JC Beyler
http://hg.openjdk.java.net/jdk/jdk/rev/083e731da31a).
    - Does it make sense to put this code in a helper method?
    - The code you added in JdbTest.java does not do the adjusted
time
Post by JC Beyler
for 30 it seems, is that normal?
Thanks,
Jc
On Fri, Oct 19, 2018 at 9:59 AM Alex Menkov
     Hi all,
     jira: https://bugs.openjdk.java.net/browse/JDK-8212151
http://cr.openjdk.java.net/~amenkov/exclusiveBind/webrev/
<http://cr.openjdk.java.net/%7Eamenkov/exclusiveBind/webrev/>
Post by JC Beyler
     <http://cr.openjdk.java.net/%7Eamenkov/exclusiveBind/webrev/>
     The fix updates the test to allow debuggee to select
available port
Post by JC Beyler
     instead of using error-prone "getFreePort" approach.
     --alex
--
Thanks,
Jc
--
Thanks,
Jc
JC Beyler
2018-10-23 02:52:51 UTC
Permalink
LGTM Alex, thanks!
Jc
Post by Alex Menkov
Hi Jc,
http://cr.openjdk.java.net/~amenkov/exclusiveBind/webrev.02/
--alex
Post by JC Beyler
Hi Alex,
It looks really good to me now. The test and JdbTest are now easier to
read because we do not have the noise of the debuggee trying to attach.
- Small nit really is the fact that the javadoc for Debuggee states
two usages and JdbTest does not use either :)
- You put a few get methods in Debuggee that are seemingly not used
anywhere, we could add them later if need be and not have them now?
Thanks for this, I think it's really better in the long run to have this
in one centralized spot,
Jc
Hi Jc,
http://cr.openjdk.java.net/~amenkov/exclusiveBind/webrev.01/
<http://cr.openjdk.java.net/%7Eamenkov/exclusiveBind/webrev.01/>
Moved shared code to new Debuggee class.
--alex
Post by JC Beyler
Hi Alex,
I remember seeing this same code so went looking for it and saw
it in
Post by JC Beyler
http://hg.openjdk.java.net/jdk/jdk/rev/083e731da31a).
- Does it make sense to put this code in a helper method?
- The code you added in JdbTest.java does not do the adjusted
time
Post by JC Beyler
for 30 it seems, is that normal?
Thanks,
Jc
On Fri, Oct 19, 2018 at 9:59 AM Alex Menkov
Hi all,
jira: https://bugs.openjdk.java.net/browse/JDK-8212151
http://cr.openjdk.java.net/~amenkov/exclusiveBind/webrev/
<http://cr.openjdk.java.net/%7Eamenkov/exclusiveBind/webrev/>
Post by JC Beyler
<http://cr.openjdk.java.net/%7Eamenkov/exclusiveBind/webrev/>
The fix updates the test to allow debuggee to select
available port
Post by JC Beyler
instead of using error-prone "getFreePort" approach.
--alex
--
Thanks,
Jc
--
Thanks,
Jc
--
Thanks,
Jc
s***@oracle.com
2018-10-22 20:58:41 UTC
Permalink
Hi Alex,

LGTM.

Thanks,
Serguei
Post by Alex Menkov
Hi Jc,
http://cr.openjdk.java.net/~amenkov/exclusiveBind/webrev.01/
Moved shared code to new Debuggee class.
--alex
Post by JC Beyler
Hi Alex,
I remember seeing this same code so went looking for it and saw it in
http://hg.openjdk.java.net/jdk/jdk/rev/083e731da31a).
   - Does it make sense to put this code in a helper method?
   - The code you added in JdbTest.java does not do the adjusted time
for 30 it seems, is that normal?
Thanks,
Jc
    Hi all,
    jira: https://bugs.openjdk.java.net/browse/JDK-8212151
    webrev: http://cr.openjdk.java.net/~amenkov/exclusiveBind/webrev/
<http://cr.openjdk.java.net/%7Eamenkov/exclusiveBind/webrev/>
    The fix updates the test to allow debuggee to select available port
    instead of using error-prone "getFreePort" approach.
    --alex
--
Thanks,
Jc
Loading...