Re: RFR : JDK-8196744 : JMX: Not enough JDP packets received before timeout

[Harsha Wardhana B]

Thanks David.

-Harsha

On Tuesday 20 March 2018 02:13 AM, David Holmes wrote:

Hi Harsha,

Given the negative nature of the test this approach seems quite 
reasonable.


Thanks,
David


Harsha Wardhana B 
Ping! Can I have one more review for the below fix?

Thanks

Harsha

On Monday 26 February 2018 10:42 AM, Harsha Wardhana B wrote:


Hello All,







Requesting for review from one more reviewer.







Thanks



Harsha







On Wednesday 21 February 2018 10:01 AM, Chris Plummer wrote:



Hi Harsha,






Not a review, but just a request that you add the explanation of the 


problem to the CR so we have a record of it. Also, the copyright 



needs to be updated.







thanks,







Chris







On 2/20/18 3:30 AM, Harsha Wardhana B wrote:



Hi All,







Please find the fix below for the Jdp test-case.







issue: https://bugs.openjdk.java.net/browse/JDK-8196028



webrev : http://cr.openjdk.java.net/~hb/8196028/webrev.00/






Fix details : The test was receiving JDP packets from other VM and 


hence the multi-cast socket was not timing-out. The default timeout 


handler was causing test to fail. Added a shutdown method that 



passes the test in case of timeout.







Thanks



Harsha

Re: RFR: 8049695: nsk/jdb/options/connect/connect003 fails with "Launched jdb could not attach to debuggee during 300000 milliseconds"

[David Holmes]

Hi Alex,

On 20/03/2018 10:28 AM, Alex Menkov wrote:

Hi guys,

please re-review the fix.


I still have an unanswered question about where the max of 49 is 
enforced. I see it for the "address" but not names in general. ??



Reg.test is added the the issue.


I don't quite follow the test. I see you try to set the name with a 
value that is too long, and if that doesn't cause an overflow and we 
don't crash that is good. But I'd expect you to read back the name and 
check it matches the truncated name with 49 characters.


Thanks,
David


webrev: http://cr.openjdk.java.net/~amenkov/shmem_long_name/webrev_open.04/

--alex

On 03/13/2018 16:14, Alex Menkov wrote:

Hi all,

Please review a small fix for
https://bugs.openjdk.java.net/browse/JDK-8049695
webrev: http://cr.openjdk.java.net/~amenkov/shmem_long_name/webrev_open/

Root cause of the issue is jbd hungs as a result of the buffer overflow.

In the beginning of the shmemBase.c:

#define MAX_IPC_PREFIX 50   /* user-specified or generated name for */
 /* shared memory seg and prefix for other 
IPC */
#define MAX_IPC_SUFFIX 25   /* suffix to shmem name for other IPC 
names */

#define MAX_IPC_NAME   (MAX_IPC_PREFIX + MAX_IPC_SUFFIX)

buffer (char prefix[]) in function createStream is used to generate 
base name for mutex/events, so MAX_IPC_PREFIX is not big enough.


--alex

RFR(S): 8195109: ServiceUtil::visible_oop is not needed anymore

[Chris Plummer]

Hello,

Please review the following:

https://bugs.openjdk.java.net/browse/JDK-8195109
http://cr.openjdk.java.net/~cjplummer/8195109/webrev.00/index.html

The assert I added to make sure this is safe has been in place in 
jdk/jdk for almost 3 weeks with no issues (longer in jdk/hs).


The webrev is missing the copyright update for threadService.hpp. I 
fixed it after noticing that.


Testing is in progress. Running hs tiers 1, 2, and 3, and jdk tiers 1 
and 2. Also making sure all serviceability tests are run.


thanks,

Chris

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[Chris Plummer]

I looked into modifying OutputAnalyzer (actually ended up being 
ProcessTools that needed all the changes) to be more flexible so it 
could support LingeredApp. The problem I ran into is that ProcessTools 
is all static, but I needed to create and return a context. It ended up 
being too much disruption, so I instead have the 
ProcessTools.getOutput() code as part of LingeredApp.


Another thing I discovered is that you can use OutputAnalyzer with 
already generated output, so this option is still available to users of 
LingeredApp. You just need to do something like:


    OutputAnalyzer out = new 
OutputAnalyzer(lingeredApp.getOutput().getStdout(), 
lingeredApp.getOutput().getStderr());


I didn't change any test to take advantage of this, but it's there if 
someone wants it.


I've included another webrev below (completely different from the 
original). In the end, all LingeredApp stdout and stderr is dumped after 
the app exits. The old way of storing away the stdout using an 
InputGobbler is gone. Since getAppOutput() depended on this, and the new 
way of saving stdout saves it as one big string rather than a List of 
lines, getAppOutput() needed some changes to convert to the List form.


http://cr.openjdk.java.net/~cjplummer/8198655/webrev.03

thanks,

Chris

On 3/19/18 9:39 AM, Chris Plummer wrote:

Hi David,

Just to clarify one point, most of the tests that use OutputAnalyzer 
do not display process output unless there is an error. So part of the 
decision here with LingeredApp is when to display the output. 
Currently the stdout is captured, but not displayed, unless the tests 
does the work to display it, which none do. Currently stderr goes to 
the console. Note that some negative tests actually cause some 
expected stderr output, although the tests don't check for it.


One thought I just had is to create an async option for OutputAnalyzer 
so it doesn't block until the process exits. Basically that means 
splitting ProcessTools.getOutput() so it doesn't block. What I 
currently have is essentially doing that. It copies 
ProcessTools.getOutput(), splitting it into two parts. But all this 
logic is in LingeredApp, and of course doesn't have any of the output 
error checking support that OutputAnalyzer, which might be useful for 
LingeredApp. For example, the negative tests only test that launching 
the app failed. They could be improved by checking for specific error 
output.


Chris

On 3/17/18 12:11 AM, David Holmes wrote:

I'm afraid I'm losing track of this change.

The key thing is that we should not have a test that launches any 
other process for which we can not see the output of that process.


David

On 17/03/2018 7:48 AM, Chris Plummer wrote:

On 3/16/18 1:25 PM, serguei.spit...@oracle.com wrote:

Hi Chris,

Thank you for taking care about this issue!

On 3/16/18 11:20, Chris Plummer wrote:

Hi,

I've resolved the issues I had before with not seeing all the 
stderr output when I tried to capture it. What I'd like to do now 
is have us decide how the output should be handled from the 
perspective a LingeredApp user (driver app). Currently all 
LingeredApp stdout is captured and gets be returned the the driver 
app by calling app.getAppOutput(). It does not appear in the .jtr 
file, but the test would have the option of dumping it there it it 
cared to. Only one test uses app.getAppOutput(). Currently all the 
LingeredApp stderr is redirected to the console, so it does not 
appear in the .jtr file.


Just a general comment to make sure I understand it and ensure we 
are in sync.
It seems much more safe to always have both stdout and stderr 
outputs present in the .jtr automatically file independently of of 
what the test does.




So how do we want this changed? Some possibilities are:

(1) capture stderr just like stdout currently is, and leave is up 
the the driver app to decide if it wants to display it (after the 
app terminates).


It does not look good to me (see above) but maybe I'm missing 
something important here.


(2) capture stderr just like stdout currently is, but have 
LingeredApp automatically send captured output to driver app's 
stdout and stderr (after the app terminates).


The stdout and std err will be separated in this case, right?
Do you have a webrev for this?
I currently have it working like this, although I need to fix 
LingeredApp.getAppOutput(). I had to make it return a single String 
instead of a List of Strings, so this breaks the one test that uses 
this API. It's easily fixed. Just haven't gotten around to it yet.



(3) send the LingeredApp's stdout and stderr to the driver app's 
stdout as it is being captured (this was the original fix Igor 
suggested and the webrev supported). A minor alternative to this 
is to keep the two streams separated instead of sending both to 
stdout.


Let me know what you think. I'm inclined to go with 2, especially 
since normally there is little to no output from the LingeredApp.


The choice (2) looks good enough.

Re: RFR : JDK-8196744 : JMX: Not enough JDP packets received before timeout

[David Holmes]

Hi Harsha,

Given the negative nature of the test this approach seems quite reasonable.

Thanks,
David

Harsha Wardhana B  


Ping! Can I have one more review for the below fix?

Thanks

Harsha

On Monday 26 February 2018 10:42 AM, Harsha Wardhana B wrote:


Hello All,







Requesting for review from one more reviewer.







Thanks



Harsha







On Wednesday 21 February 2018 10:01 AM, Chris Plummer wrote:



Hi Harsha,






Not a review, but just a request that you add the explanation of the 


problem to the CR so we have a record of it. Also, the copyright 



needs to be updated.







thanks,







Chris







On 2/20/18 3:30 AM, Harsha Wardhana B wrote:



Hi All,







Please find the fix below for the Jdp test-case.







issue: https://bugs.openjdk.java.net/browse/JDK-8196028



webrev : http://cr.openjdk.java.net/~hb/8196028/webrev.00/






Fix details : The test was receiving JDP packets from other VM and 


hence the multi-cast socket was not timing-out. The default timeout 


handler was causing test to fail. Added a shutdown method that 



passes the test in case of timeout.







Thanks



Harsha

Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr

[Chris Plummer]

Hi David,

Just to clarify one point, most of the tests that use OutputAnalyzer do 
not display process output unless there is an error. So part of the 
decision here with LingeredApp is when to display the output. Currently 
the stdout is captured, but not displayed, unless the tests does the 
work to display it, which none do. Currently stderr goes to the console. 
Note that some negative tests actually cause some expected stderr 
output, although the tests don't check for it.


One thought I just had is to create an async option for OutputAnalyzer 
so it doesn't block until the process exits. Basically that means 
splitting ProcessTools.getOutput() so it doesn't block. What I currently 
have is essentially doing that. It copies ProcessTools.getOutput(), 
splitting it into two parts. But all this logic is in LingeredApp, and 
of course doesn't have any of the output error checking support that 
OutputAnalyzer, which might be useful for LingeredApp. For example, the 
negative tests only test that launching the app failed. They could be 
improved by checking for specific error output.


Chris

On 3/17/18 12:11 AM, David Holmes wrote:

I'm afraid I'm losing track of this change.

The key thing is that we should not have a test that launches any 
other process for which we can not see the output of that process.


David

On 17/03/2018 7:48 AM, Chris Plummer wrote:

On 3/16/18 1:25 PM, serguei.spit...@oracle.com wrote:

Hi Chris,

Thank you for taking care about this issue!

On 3/16/18 11:20, Chris Plummer wrote:

Hi,

I've resolved the issues I had before with not seeing all the 
stderr output when I tried to capture it. What I'd like to do now 
is have us decide how the output should be handled from the 
perspective a LingeredApp user (driver app). Currently all 
LingeredApp stdout is captured and gets be returned the the driver 
app by calling app.getAppOutput(). It does not appear in the .jtr 
file, but the test would have the option of dumping it there it it 
cared to. Only one test uses app.getAppOutput(). Currently all the 
LingeredApp stderr is redirected to the console, so it does not 
appear in the .jtr file.


Just a general comment to make sure I understand it and ensure we 
are in sync.
It seems much more safe to always have both stdout and stderr 
outputs present in the .jtr automatically file independently of of 
what the test does.




So how do we want this changed? Some possibilities are:

(1) capture stderr just like stdout currently is, and leave is up 
the the driver app to decide if it wants to display it (after the 
app terminates).


It does not look good to me (see above) but maybe I'm missing 
something important here.


(2) capture stderr just like stdout currently is, but have 
LingeredApp automatically send captured output to driver app's 
stdout and stderr (after the app terminates).


The stdout and std err will be separated in this case, right?
Do you have a webrev for this?
I currently have it working like this, although I need to fix 
LingeredApp.getAppOutput(). I had to make it return a single String 
instead of a List of Strings, so this breaks the one test that uses 
this API. It's easily fixed. Just haven't gotten around to it yet.



(3) send the LingeredApp's stdout and stderr to the driver app's 
stdout as it is being captured (this was the original fix Igor 
suggested and the webrev supported). A minor alternative to this is 
to keep the two streams separated instead of sending both to stdout.


Let me know what you think. I'm inclined to go with 2, especially 
since normally there is little to no output from the LingeredApp.


The choice (2) looks good enough.
Not sure it is that important to have output from stdout and stderr 
sync'ed
but is is important to have the stderr present in the .jtr 
automatically.


The choice (3) looks even better if it is going to work well.
This is basically what the original webrev did. It sent LingeredApp's 
stderr and stdout to the the driver apps stdout. It's a 1 word change 
to make it send stderr to stderr. I think it has a bug though that 
did not manifest itself. It seems the new copy() code that is 
capturing stdout would be contending with the existing InputGlobbler 
code that is doing the same. I would need to fix this to make sure 
LingeredApp.getAppOutput() still returns all the apps stdout output.


Chris

Not sure, it is really necessary.

Thanks,
Serguei




BTW, here's the CR and original webrev for reference:

https://bugs.openjdk.java.net/browse/JDK-8198655
http://cr.openjdk.java.net/~cjplummer/8198655/webrev.00/webrev/

thanks,

Chris