Hi Jc,
Thank you for the review.
Initial implementation had only a single wait while outputHandler get
some new data.
But handling jdb output (actually detection of the point when jdb
completes command execution and waits for user input) is quite tricky.
So after researching existing code which works with jdb I decided to add
extra delays to minimize probability of false detection.
--alex
On 08/09/2018 11:46, JC Beyler wrote:
Hi Alexey,
I'm evidently not a reviewer but the webrev looks great to me. Since the
new classes are to be used for other tests, I was looking a bit more
into them. I saw a small nit on the waitForMsg method:
In
http://cr.openjdk.java.net/~amenkov/sh2java/webrev/test/jdk/com/sun/jdi/lib/jdb/Jdb.java.html
<http://cr.openjdk.java.net/%7Eamenkov/sh2java/webrev/test/jdk/com/sun/jdi/lib/jdb/Jdb.java.html>:
174 try {
175 Thread.sleep(sleepTime);
176 } catch (InterruptedException e) {
177 // ignore
178 }
Seems like that try/catch could be removed entirely. The next block
says: if the outputHandler is empty, wait on it for a notify. So instead
of doing a :
- Thread wait
- Check on outputHandler and wait on it
You would only check on the outputHandler and wake up when something new
came in. That might however provoke a lot of wake/notifies, on the other
hand, right now you do:
- Thread wait
- Check if there is something and wake up at the first write
- If that first write does not have the message
- Thread wait
So, then the question becomes, is it really not simpler to "just" have
the thread wait?
But apart from that (and even with it), it looks great to me :)
Thanks for doing this!
Jc
On Tue, Aug 7, 2018 at 5:09 PM Alex Menkov <[email protected]
<mailto:[email protected]>> wrote:
Hi all,
Please review a fix for
https://bugs.openjdk.java.net/browse/JDK-8209109
webrev:
http://cr.openjdk.java.net/~amenkov/sh2java/webrev/
<http://cr.openjdk.java.net/%7Eamenkov/sh2java/webrev/>
This is a first step in shell to java conversion of com/sun/jdi tests
The fix introduces base classes to work with jdb and converts couple
tests.
--alex
--
Thanks,
Jc
