Re: RFR(S) 8227435: Perf::attach() should not throw a java.lang.Exception
On 16/07/2019 9:42 pm, Langer, Christoph wrote: Hi Ralf, looks good. Prior to pushing you’ll have to take care of the copyright years in the files you touched cc-ing hotspot-runtime, because I think it affects this area, too. tl;dr - looks fine. :) I was in two minds about this. To me this is not an I/O error so the current use of Exception rather than IOException seemed more appropriate. But the Java code needs to match the VM code and tweaking the Java code to allow for the Exception would have a flow-on affect to the whole call chain. So overall treating this case as an IOException seems far simpler and not terribly wrong. Cheers, David - Thanks Christoph From: serviceability-dev On Behalf Of Schmelter, Ralf Sent: Montag, 15. Juli 2019 10:10 To: OpenJDK Serviceability Subject: [CAUTION] RFR(S) 8227435: Perf::attach() should not throw a java.lang.Exception Please review this small change. It changes the exception which will be thrown when the perf file has not yet the correct size. Instead of throwing the (not declared) java.lang.Exception, we will now throw java.io.IOException, which is expected by the calling code. webrev: http://cr.openjdk.java.net/~rschmelter/webrevs/8227435/webrev.0/ bugreport: https://bugs.openjdk.java.net/browse/JDK-8227435 Best regards, Ralf
Re: RFR(M): 8227680: FastJNIAccessors: Check for JVMTI field access event requests at runtime
Hi Martin, I need to think about this some more. A critical property of the fast field accessors are that they are trivial and completely safe. They are complicated by the need to check if a GC may have happened while we directly read the field. If you try to use fast field accessors when you have to post the field access event then how can you safely go off into a JVM TI event callback ?? Thanks, David On 16/07/2019 11:31 pm, Doerr, Martin wrote: Hi, the current implementation of FastJNIAccessors ignores the flag -XX:+UseFastJNIAccessors when the JVMTI capability "can_post_field_access" is enabled. This is an unnecessary restriction which makes field accesses (GetField) from native code slower when a JVMTI agent is attached which enables this capability. A better implementation would check at runtime if an agent actually wants to receive field access events. Note that the bytecode interpreter already uses this better implementation by checking if field access watch events were requested (JvmtiExport::_field_access_count != 0). I have implemented such a runtime check on all platforms which currently support FastJNIAccessors. My new jtreg test runtime/jni/FastGetField/FastGetField.java contains a micro benchmark: test-support/jtreg_test_hotspot_jtreg_runtime_jni_FastGetField/runtime/jni/FastGetField/FastGetField.jtr shows the duration of 1 iterations with and without UseFastJNIAccessors (JVMTI agent gets attached in both runs). My Intel(R) Xeon(R) CPU E5-2660 v3 @ 2.60GHz needed 4.7ms with FastJNIAccessors and 11.2ms without it. Webrev: http://cr.openjdk.java.net/~mdoerr/8227680_FastJNIAccessors/webrev.00/ We have run the test on 64 bit x86 platforms, SPARC and aarch64. (FastJNIAccessors are not yet available on PPC64 and s390. I'll contribute them later.) My webrev contains 32 bit implementations for x86 and arm, but completely untested. It'd be great if somebody could volunteer to review and test these platforms. Please review. Best regards, Martin
Re: 8221303: sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java fails due to java.rmi.server.ExportException: Port already in use
Hi Chris,
Yes, I added output.reportDiagnosticSummary() in webrev-01, but removed it
In webrev-02, and later restored it in webrev-03.
When the test runs of retries output.getExitValue() is set to 1
(COMMUNICATION_ERROR_EXIT_VAL)
(the "exitValue =1 " in the previous email).
Please review a new version of the fix that has an explicit error message
printed
if the test runs out of retries (line 168).
Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.04/
Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
Thanks!
--Daniil
On 7/17/19, 5:33 PM, "Chris Plummer" wrote:
Hi Daniil,
I'm confused now. I mentioned output.reportDiagnosticSummary() because I
thought I saw it in your webrev-02. Now I don't. Maybe I had glanced
back at webrev-01 and saw it there. One issue with exceptions in the
output that are not considered errors is that if later there is an
error, mdash wills show the exception as one of (possible multiple)
reasons for the failure, so it's good to avoid if possible. Looks like
what you have now is ok.
I have a question about what happens if you run out of retries. What is
output.getExitValue() set to? Also, I think you should print an an
explicit error message indicating you've run out of retries.
thanks,
Chris
On 7/17/19 4:36 PM, Daniil Titov wrote:
> Hi Chris,
>
> output.reportDiagnosticSummary() prints the output from the process (both
stdout and stderr) and the exit value to the test's stderr.
> In case if the port is already in use it prints the following:
>
> stdout: [];
> stderr: [Error: JMX connector server communication error:
service:jmx:rmi://127.0.0.1:9101
> jdk.internal.agent.AgentConfigurationError:
java.rmi.server.ExportException: Port already in use: 9101; nested exception is:
> java.net.BindException: Address already in use
> at
jdk.management.agent/sun.management.jmxremote.ConnectorBootstrap.exportMBeanServer(ConnectorBootstrap.java:820)
> at
jdk.management.agent/sun.management.jmxremote.ConnectorBootstrap.startRemoteConnectorServer(ConnectorBootstrap.java:479)
> at
jdk.management.agent/jdk.internal.agent.Agent.startAgent(Agent.java:447)
> at
jdk.management.agent/jdk.internal.agent.Agent.startAgent(Agent.java:599)
> Caused by: java.rmi.server.ExportException: Port already in use: 9101;
nested exception is:
> < I skipped the rest>
> ]
> exitValue = 1
>
> It makes sense to have it called if the test fails, otherwise this
information would be missed in the test output.
> Please review the new version of the fix that has the call to
output.reportDiagnosticSummary() restored.
>
> Thanks!
>
> Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.03/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
>
> -Daniil
>
> On 7/17/19, 3:58 PM, "Chris Plummer" wrote:
>
> What does output.reportDiagnosticSummary() print out then the port is
> already in use, and have you made this happen with your fixes in
place?
>
> Chris
>
> On 7/17/19 3:20 PM, Daniil Titov wrote:
> > Hi Chris and Alex,
> >
> > Please review a new version of the fix that moves the diagnostic
output for the test failure to run()
> > method after the number of retry attempts is exceeded. It also
includes other corrections that
> > you suggested.
> >
> > Thanks!
> >
> > Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.02/
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
> >
> > --Best regards,
> > Daniil
> >
> >
> > On 7/17/19, 1:47 PM, "Chris Plummer"
wrote:
> >
> > Hi Daniil,
> >
> > I think you can remove "Ok" from the following message:
> >
> >237 System.out.println("DEBUG: OK. Spawned java process
terminated
> > with expected exit code of "
> >238 + STOP_PROCESS_EXIT_VAL);
> >
> > It's somewhat misleading since the test can still fail. I
think the
> > following is also misleading:
> >
> >249 if (testFailed) {
> >250 output.reportDiagnosticSummary();
> >251 if (output.getStderr().contains("Port
already in
> > use")) {
> >252 // Need to retry
> >253 return true;
> >254 }
> >255 }
> >
> > The test can still pass after this happens, right? And I
think there are
> > other "Test FAILURE" error message
Re: 8221303: sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java fails due to java.rmi.server.ExportException: Port already in use
Hi Daniil,
I'm confused now. I mentioned output.reportDiagnosticSummary() because I
thought I saw it in your webrev-02. Now I don't. Maybe I had glanced
back at webrev-01 and saw it there. One issue with exceptions in the
output that are not considered errors is that if later there is an
error, mdash wills show the exception as one of (possible multiple)
reasons for the failure, so it's good to avoid if possible. Looks like
what you have now is ok.
I have a question about what happens if you run out of retries. What is
output.getExitValue() set to? Also, I think you should print an an
explicit error message indicating you've run out of retries.
thanks,
Chris
On 7/17/19 4:36 PM, Daniil Titov wrote:
Hi Chris,
output.reportDiagnosticSummary() prints the output from the process (both
stdout and stderr) and the exit value to the test's stderr.
In case if the port is already in use it prints the following:
stdout: [];
stderr: [Error: JMX connector server communication error:
service:jmx:rmi://127.0.0.1:9101
jdk.internal.agent.AgentConfigurationError: java.rmi.server.ExportException:
Port already in use: 9101; nested exception is:
java.net.BindException: Address already in use
at
jdk.management.agent/sun.management.jmxremote.ConnectorBootstrap.exportMBeanServer(ConnectorBootstrap.java:820)
at
jdk.management.agent/sun.management.jmxremote.ConnectorBootstrap.startRemoteConnectorServer(ConnectorBootstrap.java:479)
at
jdk.management.agent/jdk.internal.agent.Agent.startAgent(Agent.java:447)
at
jdk.management.agent/jdk.internal.agent.Agent.startAgent(Agent.java:599)
Caused by: java.rmi.server.ExportException: Port already in use: 9101; nested
exception is:
< I skipped the rest>
]
exitValue = 1
It makes sense to have it called if the test fails, otherwise this information
would be missed in the test output.
Please review the new version of the fix that has the call to
output.reportDiagnosticSummary() restored.
Thanks!
Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.03/
Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
-Daniil
On 7/17/19, 3:58 PM, "Chris Plummer" wrote:
What does output.reportDiagnosticSummary() print out then the port is
already in use, and have you made this happen with your fixes in place?
Chris
On 7/17/19 3:20 PM, Daniil Titov wrote:
> Hi Chris and Alex,
>
> Please review a new version of the fix that moves the diagnostic output
for the test failure to run()
> method after the number of retry attempts is exceeded. It also includes
other corrections that
> you suggested.
>
> Thanks!
>
> Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.02/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
>
> --Best regards,
> Daniil
>
>
> On 7/17/19, 1:47 PM, "Chris Plummer" wrote:
>
> Hi Daniil,
>
> I think you can remove "Ok" from the following message:
>
>237 System.out.println("DEBUG: OK. Spawned java process terminated
> with expected exit code of "
>238 + STOP_PROCESS_EXIT_VAL);
>
> It's somewhat misleading since the test can still fail. I think the
> following is also misleading:
>
>249 if (testFailed) {
>250 output.reportDiagnosticSummary();
>251 if (output.getStderr().contains("Port already
in
> use")) {
>252 // Need to retry
>253 return true;
>254 }
>255 }
>
> The test can still pass after this happens, right? And I think
there are
> other "Test FAILURE" error message that can appear just before this.
> Perhaps the code above should add a println that indicates that
there
> will be a retry attempt since the failure was due to the port being
in use.
>
> Otherwise looks good.
>
> thanks,
>
> Chris
>
> On 7/17/19 1:30 PM, Alex Menkov wrote:
> > Hi Daniil,
> >
> > The fix looks good in general.
> > Couple cosmetic notes (no new webrev required):
> >
> > 162 needRetry = runTest();
> > 163 }
> > 164 while (needRetry && (attempts++ <
MAX_RETRY_ATTEMTS));
> > Please move "while" to the prev line:
> > 163 } while (needRetry && (attempts++ <
MAX_RETRY_ATTEMTS));
> >
> >
> > 242 System.out.println("Test FAILURE on" +
name +
> > " reason: The expected line \"" + READY_MSG
> > 243 + "\" is not present in
Re: 8221303: sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java fails due to java.rmi.server.ExportException: Port already in use
Hi Chris,
output.reportDiagnosticSummary() prints the output from the process (both
stdout and stderr) and the exit value to the test's stderr.
In case if the port is already in use it prints the following:
stdout: [];
stderr: [Error: JMX connector server communication error:
service:jmx:rmi://127.0.0.1:9101
jdk.internal.agent.AgentConfigurationError: java.rmi.server.ExportException:
Port already in use: 9101; nested exception is:
java.net.BindException: Address already in use
at
jdk.management.agent/sun.management.jmxremote.ConnectorBootstrap.exportMBeanServer(ConnectorBootstrap.java:820)
at
jdk.management.agent/sun.management.jmxremote.ConnectorBootstrap.startRemoteConnectorServer(ConnectorBootstrap.java:479)
at
jdk.management.agent/jdk.internal.agent.Agent.startAgent(Agent.java:447)
at
jdk.management.agent/jdk.internal.agent.Agent.startAgent(Agent.java:599)
Caused by: java.rmi.server.ExportException: Port already in use: 9101; nested
exception is:
< I skipped the rest>
]
exitValue = 1
It makes sense to have it called if the test fails, otherwise this information
would be missed in the test output.
Please review the new version of the fix that has the call to
output.reportDiagnosticSummary() restored.
Thanks!
Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.03/
Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
-Daniil
On 7/17/19, 3:58 PM, "Chris Plummer" wrote:
What does output.reportDiagnosticSummary() print out then the port is
already in use, and have you made this happen with your fixes in place?
Chris
On 7/17/19 3:20 PM, Daniil Titov wrote:
> Hi Chris and Alex,
>
> Please review a new version of the fix that moves the diagnostic output
for the test failure to run()
> method after the number of retry attempts is exceeded. It also includes
other corrections that
> you suggested.
>
> Thanks!
>
> Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.02/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
>
> --Best regards,
> Daniil
>
>
> On 7/17/19, 1:47 PM, "Chris Plummer" wrote:
>
> Hi Daniil,
>
> I think you can remove "Ok" from the following message:
>
>237 System.out.println("DEBUG: OK. Spawned java process terminated
> with expected exit code of "
>238 + STOP_PROCESS_EXIT_VAL);
>
> It's somewhat misleading since the test can still fail. I think the
> following is also misleading:
>
>249 if (testFailed) {
>250 output.reportDiagnosticSummary();
>251 if (output.getStderr().contains("Port already
in
> use")) {
>252 // Need to retry
>253 return true;
>254 }
>255 }
>
> The test can still pass after this happens, right? And I think there
are
> other "Test FAILURE" error message that can appear just before this.
> Perhaps the code above should add a println that indicates that there
> will be a retry attempt since the failure was due to the port being
in use.
>
> Otherwise looks good.
>
> thanks,
>
> Chris
>
> On 7/17/19 1:30 PM, Alex Menkov wrote:
> > Hi Daniil,
> >
> > The fix looks good in general.
> > Couple cosmetic notes (no new webrev required):
> >
> > 162 needRetry = runTest();
> > 163 }
> > 164 while (needRetry && (attempts++ <
MAX_RETRY_ATTEMTS));
> > Please move "while" to the prev line:
> > 163 } while (needRetry && (attempts++ <
MAX_RETRY_ATTEMTS));
> >
> >
> > 242 System.out.println("Test FAILURE on" +
name +
> > " reason: The expected line \"" + READY_MSG
> > 243 + "\" is not present in the
process
> > output");
> > Please add space: "Test FAILURE on " + name
> >
> > --alex
> >
> > On 07/17/2019 12:46, Daniil Titov wrote:
> >> Hi Chris,
> >>
> >>> It's a little unclear to me why you moved from ProcessThread
to
> >>>TestProcessThread + Process. An explanation of that would
make it
> >>> easier
> >>>to understand many of the changes.
> >>
> >> There are two reasons for that:
> >> 1) For every network interface the test starts a separate thread
> >> that runs the test for this interface. We want that if the test
fails
> >> due to the bind error
Re: RFR: 8221303: sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java fails due to java.rmi.server.ExportException: Port already in use
What does output.reportDiagnosticSummary() print out then the port is
already in use, and have you made this happen with your fixes in place?
Chris
On 7/17/19 3:20 PM, Daniil Titov wrote:
Hi Chris and Alex,
Please review a new version of the fix that moves the diagnostic output for the
test failure to run()
method after the number of retry attempts is exceeded. It also includes other
corrections that
you suggested.
Thanks!
Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.02/
Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
--Best regards,
Daniil
On 7/17/19, 1:47 PM, "Chris Plummer" wrote:
Hi Daniil,
I think you can remove "Ok" from the following message:
237 System.out.println("DEBUG: OK. Spawned java process terminated
with expected exit code of "
238 + STOP_PROCESS_EXIT_VAL);
It's somewhat misleading since the test can still fail. I think the
following is also misleading:
249 if (testFailed) {
250 output.reportDiagnosticSummary();
251 if (output.getStderr().contains("Port already in
use")) {
252 // Need to retry
253 return true;
254 }
255 }
The test can still pass after this happens, right? And I think there are
other "Test FAILURE" error message that can appear just before this.
Perhaps the code above should add a println that indicates that there
will be a retry attempt since the failure was due to the port being in use.
Otherwise looks good.
thanks,
Chris
On 7/17/19 1:30 PM, Alex Menkov wrote:
> Hi Daniil,
>
> The fix looks good in general.
> Couple cosmetic notes (no new webrev required):
>
> 162 needRetry = runTest();
> 163 }
> 164 while (needRetry && (attempts++ < MAX_RETRY_ATTEMTS));
> Please move "while" to the prev line:
> 163 } while (needRetry && (attempts++ < MAX_RETRY_ATTEMTS));
>
>
> 242 System.out.println("Test FAILURE on" + name +
> " reason: The expected line \"" + READY_MSG
> 243 + "\" is not present in the process
> output");
> Please add space: "Test FAILURE on " + name
>
> --alex
>
> On 07/17/2019 12:46, Daniil Titov wrote:
>> Hi Chris,
>>
>>> It's a little unclear to me why you moved from ProcessThread to
>>>TestProcessThread + Process. An explanation of that would make it
>>> easier
>>>to understand many of the changes.
>>
>> There are two reasons for that:
>> 1) For every network interface the test starts a separate thread
>> that runs the test for this interface. We want that if the test fails
>> due to the bind error the thread not to exit but try to repeat
>> the test several times. jdk.test.lib.thread.ProcessThread doesn't
>> allow this.
>> 2) To filter out the cases when the test fails due to the bind error
>> we need to parse the process output. jdk.test.lib.thread.ProcessThread
>>registers its own pumps for stdin and sdtout (by calling
>> ProcessTools.startProcess()) that consume all output of the process
>> and prevent
>>the output analyzer from collecting this output.
>> Thanks!
>> --Daniil
>>
>> On 7/17/19, 12:11 PM, "Chris Plummer" wrote:
>>
>> Hi Daniil,
>> It's a little unclear to me why you moved from
>> ProcessThread to
>> TestProcessThread + Process. An explanation of that would make
>> it easier
>> to understand many of the changes.
>> thanks,
>> Chris
>> On 7/11/19 10:16 AM, Daniil Titov wrote:
>> > Please review the change that fixes an intermittent failure of
>> sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java
>> > test due to ports collision. The tests finds all network
>> interfaces and for every interface starts a separate process that tests
>> > the connection to JMX agent server for a specific
>> ports/interface combination.
>> >
>> > The test was changed to retry in case of the failure. If the
>> subprocess fails to bind and the number
>> > of retry attempts doesn't exceed the limit a new pair of
>> random ports is selected and the test is run again.
>> >
>> > Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.01/
>> > Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
>> >
>> > Thanks!
>> > --Daniil
>> >
>> >
>>
>>
Re: RFR: 8221303: sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java fails due to java.rmi.server.ExportException: Port already in use
Hi Chris and Alex,
Please review a new version of the fix that moves the diagnostic output for the
test failure to run()
method after the number of retry attempts is exceeded. It also includes other
corrections that
you suggested.
Thanks!
Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.02/
Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
--Best regards,
Daniil
On 7/17/19, 1:47 PM, "Chris Plummer" wrote:
Hi Daniil,
I think you can remove "Ok" from the following message:
237 System.out.println("DEBUG: OK. Spawned java process terminated
with expected exit code of "
238 + STOP_PROCESS_EXIT_VAL);
It's somewhat misleading since the test can still fail. I think the
following is also misleading:
249 if (testFailed) {
250 output.reportDiagnosticSummary();
251 if (output.getStderr().contains("Port already in
use")) {
252 // Need to retry
253 return true;
254 }
255 }
The test can still pass after this happens, right? And I think there are
other "Test FAILURE" error message that can appear just before this.
Perhaps the code above should add a println that indicates that there
will be a retry attempt since the failure was due to the port being in use.
Otherwise looks good.
thanks,
Chris
On 7/17/19 1:30 PM, Alex Menkov wrote:
> Hi Daniil,
>
> The fix looks good in general.
> Couple cosmetic notes (no new webrev required):
>
> 162 needRetry = runTest();
> 163 }
> 164 while (needRetry && (attempts++ < MAX_RETRY_ATTEMTS));
> Please move "while" to the prev line:
> 163 } while (needRetry && (attempts++ < MAX_RETRY_ATTEMTS));
>
>
> 242 System.out.println("Test FAILURE on" + name +
> " reason: The expected line \"" + READY_MSG
> 243 + "\" is not present in the process
> output");
> Please add space: "Test FAILURE on " + name
>
> --alex
>
> On 07/17/2019 12:46, Daniil Titov wrote:
>> Hi Chris,
>>
>>> It's a little unclear to me why you moved from ProcessThread to
>>>TestProcessThread + Process. An explanation of that would make it
>>> easier
>>>to understand many of the changes.
>>
>> There are two reasons for that:
>> 1) For every network interface the test starts a separate thread
>> that runs the test for this interface. We want that if the test fails
>> due to the bind error the thread not to exit but try to repeat
>> the test several times. jdk.test.lib.thread.ProcessThread doesn't
>> allow this.
>> 2) To filter out the cases when the test fails due to the bind error
>> we need to parse the process output. jdk.test.lib.thread.ProcessThread
>>registers its own pumps for stdin and sdtout (by calling
>> ProcessTools.startProcess()) that consume all output of the process
>> and prevent
>>the output analyzer from collecting this output.
>> Thanks!
>> --Daniil
>>
>> On 7/17/19, 12:11 PM, "Chris Plummer" wrote:
>>
>> Hi Daniil,
>> It's a little unclear to me why you moved from
>> ProcessThread to
>> TestProcessThread + Process. An explanation of that would make
>> it easier
>> to understand many of the changes.
>> thanks,
>> Chris
>> On 7/11/19 10:16 AM, Daniil Titov wrote:
>> > Please review the change that fixes an intermittent failure of
>> sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java
>> > test due to ports collision. The tests finds all network
>> interfaces and for every interface starts a separate process that tests
>> > the connection to JMX agent server for a specific
>> ports/interface combination.
>> >
>> > The test was changed to retry in case of the failure. If the
>> subprocess fails to bind and the number
>> > of retry attempts doesn't exceed the limit a new pair of
>> random ports is selected and the test is run again.
>> >
>> > Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.01/
>> > Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
>> >
>> > Thanks!
>> > --Daniil
>> >
>> >
>>
>>
Re: jmx-dev 8221303: sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java fails due to java.rmi.server.ExportException: Port already in use
Hi Daniil,
I think you can remove "Ok" from the following message:
237 System.out.println("DEBUG: OK. Spawned java process terminated
with expected exit code of "
238 + STOP_PROCESS_EXIT_VAL);
It's somewhat misleading since the test can still fail. I think the
following is also misleading:
249 if (testFailed) {
250 output.reportDiagnosticSummary();
251 if (output.getStderr().contains("Port already in
use")) {
252 // Need to retry
253 return true;
254 }
255 }
The test can still pass after this happens, right? And I think there are
other "Test FAILURE" error message that can appear just before this.
Perhaps the code above should add a println that indicates that there
will be a retry attempt since the failure was due to the port being in use.
Otherwise looks good.
thanks,
Chris
On 7/17/19 1:30 PM, Alex Menkov wrote:
Hi Daniil,
The fix looks good in general.
Couple cosmetic notes (no new webrev required):
162 needRetry = runTest();
163 }
164 while (needRetry && (attempts++ < MAX_RETRY_ATTEMTS));
Please move "while" to the prev line:
163 } while (needRetry && (attempts++ < MAX_RETRY_ATTEMTS));
242 System.out.println("Test FAILURE on" + name +
" reason: The expected line \"" + READY_MSG
243 + "\" is not present in the process
output");
Please add space: "Test FAILURE on " + name
--alex
On 07/17/2019 12:46, Daniil Titov wrote:
Hi Chris,
It's a little unclear to me why you moved from ProcessThread to
TestProcessThread + Process. An explanation of that would make it
easier
to understand many of the changes.
There are two reasons for that:
1) For every network interface the test starts a separate thread
that runs the test for this interface. We want that if the test fails
due to the bind error the thread not to exit but try to repeat
the test several times. jdk.test.lib.thread.ProcessThread doesn't
allow this.
2) To filter out the cases when the test fails due to the bind error
we need to parse the process output. jdk.test.lib.thread.ProcessThread
registers its own pumps for stdin and sdtout (by calling
ProcessTools.startProcess()) that consume all output of the process
and prevent
the output analyzer from collecting this output.
Thanks!
--Daniil
On 7/17/19, 12:11 PM, "Chris Plummer" wrote:
Hi Daniil,
It's a little unclear to me why you moved from
ProcessThread to
TestProcessThread + Process. An explanation of that would make
it easier
to understand many of the changes.
thanks,
Chris
On 7/11/19 10:16 AM, Daniil Titov wrote:
> Please review the change that fixes an intermittent failure of
sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java
> test due to ports collision. The tests finds all network
interfaces and for every interface starts a separate process that tests
> the connection to JMX agent server for a specific
ports/interface combination.
>
> The test was changed to retry in case of the failure. If the
subprocess fails to bind and the number
> of retry attempts doesn't exceed the limit a new pair of
random ports is selected and the test is run again.
>
> Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.01/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
>
> Thanks!
> --Daniil
>
>
Re: jmx-dev 8221303: sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java fails due to java.rmi.server.ExportException: Port already in use
Hi Daniil,
The fix looks good in general.
Couple cosmetic notes (no new webrev required):
162 needRetry = runTest();
163 }
164 while (needRetry && (attempts++ < MAX_RETRY_ATTEMTS));
Please move "while" to the prev line:
163 } while (needRetry && (attempts++ < MAX_RETRY_ATTEMTS));
242 System.out.println("Test FAILURE on" + name +
" reason: The expected line \"" + READY_MSG
243 + "\" is not present in the process
output");
Please add space: "Test FAILURE on " + name
--alex
On 07/17/2019 12:46, Daniil Titov wrote:
Hi Chris,
It's a little unclear to me why you moved from ProcessThread to
TestProcessThread + Process. An explanation of that would make it easier
to understand many of the changes.
There are two reasons for that:
1) For every network interface the test starts a separate thread that runs the
test for this interface. We want that if the test fails
due to the bind error the thread not to exit but try to repeat the test
several times. jdk.test.lib.thread.ProcessThread doesn't allow this.
2) To filter out the cases when the test fails due to the bind error we need to
parse the process output. jdk.test.lib.thread.ProcessThread
registers its own pumps for stdin and sdtout (by calling
ProcessTools.startProcess()) that consume all output of the process and prevent
the output analyzer from collecting this output.
Thanks!
--Daniil
On 7/17/19, 12:11 PM, "Chris Plummer" wrote:
Hi Daniil,
It's a little unclear to me why you moved from ProcessThread to
TestProcessThread + Process. An explanation of that would make it easier
to understand many of the changes.
thanks,
Chris
On 7/11/19 10:16 AM, Daniil Titov wrote:
> Please review the change that fixes an intermittent failure of
sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java
> test due to ports collision. The tests finds all network interfaces and
for every interface starts a separate process that tests
> the connection to JMX agent server for a specific ports/interface
combination.
>
> The test was changed to retry in case of the failure. If the subprocess
fails to bind and the number
> of retry attempts doesn't exceed the limit a new pair of random ports is
selected and the test is run again.
>
> Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.01/
> Bug: https://bugs.openjdk.java.net/browse/JDK-8221303
>
> Thanks!
> --Daniil
>
>
Re: 8221303: sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java fails due to java.rmi.server.ExportException: Port already in use
Hi Chris, >It's a little unclear to me why you moved from ProcessThread to > TestProcessThread + Process. An explanation of that would make it easier > to understand many of the changes. There are two reasons for that: 1) For every network interface the test starts a separate thread that runs the test for this interface. We want that if the test fails due to the bind error the thread not to exit but try to repeat the test several times. jdk.test.lib.thread.ProcessThread doesn't allow this. 2) To filter out the cases when the test fails due to the bind error we need to parse the process output. jdk.test.lib.thread.ProcessThread registers its own pumps for stdin and sdtout (by calling ProcessTools.startProcess()) that consume all output of the process and prevent the output analyzer from collecting this output. Thanks! --Daniil On 7/17/19, 12:11 PM, "Chris Plummer" wrote: Hi Daniil, It's a little unclear to me why you moved from ProcessThread to TestProcessThread + Process. An explanation of that would make it easier to understand many of the changes. thanks, Chris On 7/11/19 10:16 AM, Daniil Titov wrote: > Please review the change that fixes an intermittent failure of sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java > test due to ports collision. The tests finds all network interfaces and for every interface starts a separate process that tests > the connection to JMX agent server for a specific ports/interface combination. > > The test was changed to retry in case of the failure. If the subprocess fails to bind and the number > of retry attempts doesn't exceed the limit a new pair of random ports is selected and the test is run again. > > Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.01/ > Bug: https://bugs.openjdk.java.net/browse/JDK-8221303 > > Thanks! > --Daniil > >
Re: RFR: 8221303: sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java fails due to java.rmi.server.ExportException: Port already in use
Hi Daniil, It's a little unclear to me why you moved from ProcessThread to TestProcessThread + Process. An explanation of that would make it easier to understand many of the changes. thanks, Chris On 7/11/19 10:16 AM, Daniil Titov wrote: Please review the change that fixes an intermittent failure of sun/management/jmxremote/bootstrap/JMXInterfaceBindingTest.java test due to ports collision. The tests finds all network interfaces and for every interface starts a separate process that tests the connection to JMX agent server for a specific ports/interface combination. The test was changed to retry in case of the failure. If the subprocess fails to bind and the number of retry attempts doesn't exceed the limit a new pair of random ports is selected and the test is run again. Webrev: http://cr.openjdk.java.net/~dtitov/8221303/webrev.01/ Bug: https://bugs.openjdk.java.net/browse/JDK-8221303 Thanks! --Daniil
RFR: 8226575: OperatingSystemMXBean should be made container aware
Hi all, Please review this change that addresses JDK-8226575 according to a previous discussion on this list [0][1]. The webrev has been kindly uploaded on my behalf by Severin Gehwolf, since I am not an author. The initial problem was that the com.sun.management.OperatingSystemMXBean was inconsistent about its awareness of applicable container limits. On the one hand, the (inherited) getAvailableProcessors() return value is consistent with the container-aware Runtime.availableProcessors(). But on the other hand, c.s.m.OperatingSystemMXBean's getTotalPhysicalMemorySize(), getFreePhysicalMemorySize(), getTotalSwapSpaceSize(), and getFreeSwapSpaceSize() returned values reflecting the physical host machine with no awareness of container limits. getSystemCpuLoad() has also been updated to use cgroup-accounted load calculation. The fix applied is to use the JDK internal Metrics API to determine container memory limits and CPU accounting and use these values instead, if available, otherwise falling back on the pre-existing native implementations. bug: https://bugs.openjdk.java.net/browse/JDK-8226575 webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/aazores/JDK-8226575/01/webrev/ [0] http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-June/028439.html [1] http://mail.openjdk.java.net/pipermail/serviceability-dev/2019-July/028709.html Thanks, -- Andrew Azores Software Engineer, OpenJDK Team Red Hat
Re: 8206179: com/sun/management/OperatingSystemMXBean/GetCommittedVirtualMemorySize.java fails with Committed virtual memory size illegal value
Thank you, Chris and Serguei, for reviewing this change! Best regards, Daniil On 7/16/19, 4:58 PM, "Chris Plummer" wrote: Looks good. Chris On 7/16/19 4:12 PM, Daniil Titov wrote: > Please review the change that fixes the failure of the test. > > The test assumes that the amount of the virtual memory committed to the process could not > be greater than the total of the RAM and the swap size the machine has, however, it is not > correct if, for example, the process uses a memory-mapped file. > > That results in the test failure on the machines where the total of the RAM and the size of the swap > file is less than the memory committed to the test process. > > Webrev: https://cr.openjdk.java.net/~dtitov/8206179/webrev.01/ > Bug: https://bugs.openjdk.java.net/browse/JDK-8206179 > > Thanks, > --Daniil > >
Re: RFR: 8227815: Minimal VM: set_state is not a member of AttachListener
Looks good. Chris On 7/17/19 7:37 AM, Yasumasa Suenaga wrote: Hi all, Please review this change: JBS: https://bugs.openjdk.java.net/browse/JDK-8227815 webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8227815/webrev.00/ After JDK-8225690, minimal VM could not be built. Minimal VM does not include Attach Listener implementation. So I exclude it in os.cpp with INCLUDE_SERVICES macro. Thanks, Yasumasa
RFR: 8227815: Minimal VM: set_state is not a member of AttachListener
Hi all, Please review this change: JBS: https://bugs.openjdk.java.net/browse/JDK-8227815 webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8227815/webrev.00/ After JDK-8225690, minimal VM could not be built. Minimal VM does not include Attach Listener implementation. So I exclude it in os.cpp with INCLUDE_SERVICES macro. Thanks, Yasumasa
