Re: RFR : JDK-8196744 : JMX: Not enough JDP packets received before timeout
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"
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
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: 8049695: nsk/jdb/options/connect/connect003 fails with "Launched jdb could not attach to debuggee during 300000 milliseconds"
Hi guys, please re-review the fix. Reg.test is added the the issue. 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
Re: RFR(S): 8198655: test/lib/jdk/test/lib/apps/LingeredApp shouldn't inherit cout/cerr
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. N
Re: RFR : JDK-8196744 : JMX: Not enough JDP packets received before timeout
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
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