Re: RFR of JDK-8188897: java/rmi/registry/reexport/Reexport.java failed with Port already in use
Hi Hamlin, Looks good, Thanks for the updates. On 4/9/2018 10:49 PM, Hamlin Li wrote: On 10/04/2018 3:09 AM, Roger Riggs wrote: Hi Hamlin, Hi Roger, Thank you for review. Great, the new busy port algorithm looks better. The port assigned will always be one that was available and is now busy to prevent the registry creation. As expected, there is a small window between the (try-with-finally) close of the server socket channel and the 2nd createReg. But that can't be avoided. If the port is re-used in that gap the test will fail. (And the exception handler at 77 will see an in-use and retry). Yes, you're right. 67: The exception being caught is already one thrown by TestLibrary.bomb so it would be cleaner to just re-throw e; or better yet, just don't bother to catch the exception. That exception should cause the test to fail. It's to catch IOException by ServerSocketChannel.open, bind. Seems it's a little confusing, so I declare at main function to throw IOException. It may be personal coding style but the createReg method with a boolean flag to suppress the exception is just more confusing that putting the code in-line in the two places it is used. I think it's mainly because of original parameter name remoteOK, so rename it as expectException, and move it to the last parameter. new webrev is updated in place: http://cr.openjdk.java.net/~mli/8188897/webrev.02/ Thank you -Hamlin Thanks, Roger On 4/8/2018 4:10 AM, Hamlin Li wrote: Hi Roger, I have changed to not use RegistryVM at all, please review the patch: http://cr.openjdk.java.net/~mli/8188897/webrev.02/ Thank you -Hamlin On 04/04/2018 10:15 PM, Roger Riggs wrote: Hi Hamlin, Reexport.java: I think this change to use a separate process for the 2nd registry changes the test so that it does not address the original test case. The original problem was the incorrect retention of an object in the object table when the create of a registry in the same process failed. Finally being able to create the registry in the same process assured that the object was not retained in the object table. Go back to creating the 2nd registry in the test process. RegistryVM.java: 2, the copyright update should be "2017, 2018," range. (I'm really not a fan of all the RegistryVM methods with the same name and minimal and implicit differences in their functions. When reading the test, you have to go and read the RegistryVM method to see what it does. I would have preferred that the full createRegistryVM (out, err, options, port) method was used directly in the tests. In the case of a method used once, it is an inconvenience method, not a convenience). The new terminate() method is quite similar to the existing cleanup() method which does not wait. It would be a good cleanup to figure out if another method is really needed. The override is going to change the behavior of the existing uses of terminate(). It should be checked that it does not break any existing uses. 178: 187: The method comments are not consistent, one says forcibly and the other gracefully but both call requestExit and both call destroy(). Thanks, Roger On 4/3/2018 11:35 PM, Hamlin Li wrote: Hi Joe, Roger, Thank you for reviewing, I have refactored the test more and fix as you suggested. please review http://cr.openjdk.java.net/~mli/8188897/webrev.01/ Thank you -Hamlin On 04/04/2018 10:42 AM, Joseph D. Darcy wrote: Hello, Some general comments on the coding for tests like this: * It is preferable to avoid sleep in tests to avoid increasing the minimum amount of time a test takes to run. This helps limit the overall time it takes the test suite to run. * If timeouts are used, it is recommend to factor the maximum time waited with the jtreg timeout scaling factor; I don't recall its exact name. In other words, many of our tests are run on heavily loaded systems and the tests take longer than run than on typical laptops and workstations so jtreg is invoked with an timeout scaling factor. Individual tests should be sensitive to this scaling factor for any internal timeout that need to be used. HTH, -Joe On 4/3/2018 7:48 AM, Roger Riggs wrote: Hi Hamlin, Instead of a simple time delay, it would be useful to wait for the RegistryVM to terminate. In killRegistry: 149, adding subreg.waitFor() should be sufficient. 58: If using a 'for' loop it would be easier to understand if it included the usual start, increment and termination. Instead of burying it in the exception handler. 59, 102, 104: the introduction of the kill boolean makes the test harder to understand and seems to be unnecessary. the killRegistry() method already will only kill the subprocess if it still is alive. Roger On 4/2/2018 6:33 AM, Hamlin Li wrote: would you please review the following patch? bug: https://bugs.openjdk.java.net/browse/JDK-8188897 webrev: http://cr.openjdk.java.net/~mli/8188897/webrev.00/ Thank you -Hamlin
Re: RFR of JDK-8188897: java/rmi/registry/reexport/Reexport.java failed with Port already in use
On 10/04/2018 3:09 AM, Roger Riggs wrote: Hi Hamlin, Hi Roger, Thank you for review. Great, the new busy port algorithm looks better. The port assigned will always be one that was available and is now busy to prevent the registry creation. As expected, there is a small window between the (try-with-finally) close of the server socket channel and the 2nd createReg. But that can't be avoided. If the port is re-used in that gap the test will fail. (And the exception handler at 77 will see an in-use and retry). Yes, you're right. 67: The exception being caught is already one thrown by TestLibrary.bomb so it would be cleaner to just re-throw e; or better yet, just don't bother to catch the exception. That exception should cause the test to fail. It's to catch IOException by ServerSocketChannel.open, bind. Seems it's a little confusing, so I declare at main function to throw IOException. It may be personal coding style but the createReg method with a boolean flag to suppress the exception is just more confusing that putting the code in-line in the two places it is used. I think it's mainly because of original parameter name remoteOK, so rename it as expectException, and move it to the last parameter. new webrev is updated in place: http://cr.openjdk.java.net/~mli/8188897/webrev.02/ Thank you -Hamlin Thanks, Roger On 4/8/2018 4:10 AM, Hamlin Li wrote: Hi Roger, I have changed to not use RegistryVM at all, please review the patch: http://cr.openjdk.java.net/~mli/8188897/webrev.02/ Thank you -Hamlin On 04/04/2018 10:15 PM, Roger Riggs wrote: Hi Hamlin, Reexport.java: I think this change to use a separate process for the 2nd registry changes the test so that it does not address the original test case. The original problem was the incorrect retention of an object in the object table when the create of a registry in the same process failed. Finally being able to create the registry in the same process assured that the object was not retained in the object table. Go back to creating the 2nd registry in the test process. RegistryVM.java: 2, the copyright update should be "2017, 2018," range. (I'm really not a fan of all the RegistryVM methods with the same name and minimal and implicit differences in their functions. When reading the test, you have to go and read the RegistryVM method to see what it does. I would have preferred that the full createRegistryVM (out, err, options, port) method was used directly in the tests. In the case of a method used once, it is an inconvenience method, not a convenience). The new terminate() method is quite similar to the existing cleanup() method which does not wait. It would be a good cleanup to figure out if another method is really needed. The override is going to change the behavior of the existing uses of terminate(). It should be checked that it does not break any existing uses. 178: 187: The method comments are not consistent, one says forcibly and the other gracefully but both call requestExit and both call destroy(). Thanks, Roger On 4/3/2018 11:35 PM, Hamlin Li wrote: Hi Joe, Roger, Thank you for reviewing, I have refactored the test more and fix as you suggested. please review http://cr.openjdk.java.net/~mli/8188897/webrev.01/ Thank you -Hamlin On 04/04/2018 10:42 AM, Joseph D. Darcy wrote: Hello, Some general comments on the coding for tests like this: * It is preferable to avoid sleep in tests to avoid increasing the minimum amount of time a test takes to run. This helps limit the overall time it takes the test suite to run. * If timeouts are used, it is recommend to factor the maximum time waited with the jtreg timeout scaling factor; I don't recall its exact name. In other words, many of our tests are run on heavily loaded systems and the tests take longer than run than on typical laptops and workstations so jtreg is invoked with an timeout scaling factor. Individual tests should be sensitive to this scaling factor for any internal timeout that need to be used. HTH, -Joe On 4/3/2018 7:48 AM, Roger Riggs wrote: Hi Hamlin, Instead of a simple time delay, it would be useful to wait for the RegistryVM to terminate. In killRegistry: 149, adding subreg.waitFor() should be sufficient. 58: If using a 'for' loop it would be easier to understand if it included the usual start, increment and termination. Instead of burying it in the exception handler. 59, 102, 104: the introduction of the kill boolean makes the test harder to understand and seems to be unnecessary. the killRegistry() method already will only kill the subprocess if it still is alive. Roger On 4/2/2018 6:33 AM, Hamlin Li wrote: would you please review the following patch? bug: https://bugs.openjdk.java.net/browse/JDK-8188897 webrev: http://cr.openjdk.java.net/~mli/8188897/webrev.00/ Thank you -Hamlin
Re: RFR of JDK-8188897: java/rmi/registry/reexport/Reexport.java failed with Port already in use
Hi Hamlin, Great, the new busy port algorithm looks better. The port assigned will always be one that was available and is now busy to prevent the registry creation. As expected, there is a small window between the (try-with-finally) close of the server socket channel and the 2nd createReg. But that can't be avoided. If the port is re-used in that gap the test will fail. (And the exception handler at 77 will see an in-use and retry). 67: The exception being caught is already one thrown by TestLibrary.bomb so it would be cleaner to just re-throw e; or better yet, just don't bother to catch the exception. That exception should cause the test to fail. It may be personal coding style but the createReg method with a boolean flag to suppress the exception is just more confusing that putting the code in-line in the two places it is used. Thanks, Roger On 4/8/2018 4:10 AM, Hamlin Li wrote: Hi Roger, I have changed to not use RegistryVM at all, please review the patch: http://cr.openjdk.java.net/~mli/8188897/webrev.02/ Thank you -Hamlin On 04/04/2018 10:15 PM, Roger Riggs wrote: Hi Hamlin, Reexport.java: I think this change to use a separate process for the 2nd registry changes the test so that it does not address the original test case. The original problem was the incorrect retention of an object in the object table when the create of a registry in the same process failed. Finally being able to create the registry in the same process assured that the object was not retained in the object table. Go back to creating the 2nd registry in the test process. RegistryVM.java: 2, the copyright update should be "2017, 2018," range. (I'm really not a fan of all the RegistryVM methods with the same name and minimal and implicit differences in their functions. When reading the test, you have to go and read the RegistryVM method to see what it does. I would have preferred that the full createRegistryVM (out, err, options, port) method was used directly in the tests. In the case of a method used once, it is an inconvenience method, not a convenience). The new terminate() method is quite similar to the existing cleanup() method which does not wait. It would be a good cleanup to figure out if another method is really needed. The override is going to change the behavior of the existing uses of terminate(). It should be checked that it does not break any existing uses. 178: 187: The method comments are not consistent, one says forcibly and the other gracefully but both call requestExit and both call destroy(). Thanks, Roger On 4/3/2018 11:35 PM, Hamlin Li wrote: Hi Joe, Roger, Thank you for reviewing, I have refactored the test more and fix as you suggested. please review http://cr.openjdk.java.net/~mli/8188897/webrev.01/ Thank you -Hamlin On 04/04/2018 10:42 AM, Joseph D. Darcy wrote: Hello, Some general comments on the coding for tests like this: * It is preferable to avoid sleep in tests to avoid increasing the minimum amount of time a test takes to run. This helps limit the overall time it takes the test suite to run. * If timeouts are used, it is recommend to factor the maximum time waited with the jtreg timeout scaling factor; I don't recall its exact name. In other words, many of our tests are run on heavily loaded systems and the tests take longer than run than on typical laptops and workstations so jtreg is invoked with an timeout scaling factor. Individual tests should be sensitive to this scaling factor for any internal timeout that need to be used. HTH, -Joe On 4/3/2018 7:48 AM, Roger Riggs wrote: Hi Hamlin, Instead of a simple time delay, it would be useful to wait for the RegistryVM to terminate. In killRegistry: 149, adding subreg.waitFor() should be sufficient. 58: If using a 'for' loop it would be easier to understand if it included the usual start, increment and termination. Instead of burying it in the exception handler. 59, 102, 104: the introduction of the kill boolean makes the test harder to understand and seems to be unnecessary. the killRegistry() method already will only kill the subprocess if it still is alive. Roger On 4/2/2018 6:33 AM, Hamlin Li wrote: would you please review the following patch? bug: https://bugs.openjdk.java.net/browse/JDK-8188897 webrev: http://cr.openjdk.java.net/~mli/8188897/webrev.00/ Thank you -Hamlin
Re: RFR of JDK-8188897: java/rmi/registry/reexport/Reexport.java failed with Port already in use
Hi Roger, I have changed to not use RegistryVM at all, please review the patch: http://cr.openjdk.java.net/~mli/8188897/webrev.02/ Thank you -Hamlin On 04/04/2018 10:15 PM, Roger Riggs wrote: Hi Hamlin, Reexport.java: I think this change to use a separate process for the 2nd registry changes the test so that it does not address the original test case. The original problem was the incorrect retention of an object in the object table when the create of a registry in the same process failed. Finally being able to create the registry in the same process assured that the object was not retained in the object table. Go back to creating the 2nd registry in the test process. RegistryVM.java: 2, the copyright update should be "2017, 2018," range. (I'm really not a fan of all the RegistryVM methods with the same name and minimal and implicit differences in their functions. When reading the test, you have to go and read the RegistryVM method to see what it does. I would have preferred that the full createRegistryVM (out, err, options, port) method was used directly in the tests. In the case of a method used once, it is an inconvenience method, not a convenience). The new terminate() method is quite similar to the existing cleanup() method which does not wait. It would be a good cleanup to figure out if another method is really needed. The override is going to change the behavior of the existing uses of terminate(). It should be checked that it does not break any existing uses. 178: 187: The method comments are not consistent, one says forcibly and the other gracefully but both call requestExit and both call destroy(). Thanks, Roger On 4/3/2018 11:35 PM, Hamlin Li wrote: Hi Joe, Roger, Thank you for reviewing, I have refactored the test more and fix as you suggested. please review http://cr.openjdk.java.net/~mli/8188897/webrev.01/ Thank you -Hamlin On 04/04/2018 10:42 AM, Joseph D. Darcy wrote: Hello, Some general comments on the coding for tests like this: * It is preferable to avoid sleep in tests to avoid increasing the minimum amount of time a test takes to run. This helps limit the overall time it takes the test suite to run. * If timeouts are used, it is recommend to factor the maximum time waited with the jtreg timeout scaling factor; I don't recall its exact name. In other words, many of our tests are run on heavily loaded systems and the tests take longer than run than on typical laptops and workstations so jtreg is invoked with an timeout scaling factor. Individual tests should be sensitive to this scaling factor for any internal timeout that need to be used. HTH, -Joe On 4/3/2018 7:48 AM, Roger Riggs wrote: Hi Hamlin, Instead of a simple time delay, it would be useful to wait for the RegistryVM to terminate. In killRegistry: 149, adding subreg.waitFor() should be sufficient. 58: If using a 'for' loop it would be easier to understand if it included the usual start, increment and termination. Instead of burying it in the exception handler. 59, 102, 104: the introduction of the kill boolean makes the test harder to understand and seems to be unnecessary. the killRegistry() method already will only kill the subprocess if it still is alive. Roger On 4/2/2018 6:33 AM, Hamlin Li wrote: would you please review the following patch? bug: https://bugs.openjdk.java.net/browse/JDK-8188897 webrev: http://cr.openjdk.java.net/~mli/8188897/webrev.00/ Thank you -Hamlin
Re: RFR of JDK-8188897: java/rmi/registry/reexport/Reexport.java failed with Port already in use
Hi Hamlin, Reexport.java: I think this change to use a separate process for the 2nd registry changes the test so that it does not address the original test case. The original problem was the incorrect retention of an object in the object table when the create of a registry in the same process failed. Finally being able to create the registry in the same process assured that the object was not retained in the object table. Go back to creating the 2nd registry in the test process. RegistryVM.java: 2, the copyright update should be "2017, 2018," range. (I'm really not a fan of all the RegistryVM methods with the same name and minimal and implicit differences in their functions. When reading the test, you have to go and read the RegistryVM method to see what it does. I would have preferred that the full createRegistryVM (out, err, options, port) method was used directly in the tests. In the case of a method used once, it is an inconvenience method, not a convenience). The new terminate() method is quite similar to the existing cleanup() method which does not wait. It would be a good cleanup to figure out if another method is really needed. The override is going to change the behavior of the existing uses of terminate(). It should be checked that it does not break any existing uses. 178: 187: The method comments are not consistent, one says forcibly and the other gracefully but both call requestExit and both call destroy(). Thanks, Roger On 4/3/2018 11:35 PM, Hamlin Li wrote: Hi Joe, Roger, Thank you for reviewing, I have refactored the test more and fix as you suggested. please review http://cr.openjdk.java.net/~mli/8188897/webrev.01/ Thank you -Hamlin On 04/04/2018 10:42 AM, Joseph D. Darcy wrote: Hello, Some general comments on the coding for tests like this: * It is preferable to avoid sleep in tests to avoid increasing the minimum amount of time a test takes to run. This helps limit the overall time it takes the test suite to run. * If timeouts are used, it is recommend to factor the maximum time waited with the jtreg timeout scaling factor; I don't recall its exact name. In other words, many of our tests are run on heavily loaded systems and the tests take longer than run than on typical laptops and workstations so jtreg is invoked with an timeout scaling factor. Individual tests should be sensitive to this scaling factor for any internal timeout that need to be used. HTH, -Joe On 4/3/2018 7:48 AM, Roger Riggs wrote: Hi Hamlin, Instead of a simple time delay, it would be useful to wait for the RegistryVM to terminate. In killRegistry: 149, adding subreg.waitFor() should be sufficient. 58: If using a 'for' loop it would be easier to understand if it included the usual start, increment and termination. Instead of burying it in the exception handler. 59, 102, 104: the introduction of the kill boolean makes the test harder to understand and seems to be unnecessary. the killRegistry() method already will only kill the subprocess if it still is alive. Roger On 4/2/2018 6:33 AM, Hamlin Li wrote: would you please review the following patch? bug: https://bugs.openjdk.java.net/browse/JDK-8188897 webrev: http://cr.openjdk.java.net/~mli/8188897/webrev.00/ Thank you -Hamlin
Re: RFR of JDK-8188897: java/rmi/registry/reexport/Reexport.java failed with Port already in use
Hi Joe, Roger, Thank you for reviewing, I have refactored the test more and fix as you suggested. please review http://cr.openjdk.java.net/~mli/8188897/webrev.01/ Thank you -Hamlin On 04/04/2018 10:42 AM, Joseph D. Darcy wrote: Hello, Some general comments on the coding for tests like this: * It is preferable to avoid sleep in tests to avoid increasing the minimum amount of time a test takes to run. This helps limit the overall time it takes the test suite to run. * If timeouts are used, it is recommend to factor the maximum time waited with the jtreg timeout scaling factor; I don't recall its exact name. In other words, many of our tests are run on heavily loaded systems and the tests take longer than run than on typical laptops and workstations so jtreg is invoked with an timeout scaling factor. Individual tests should be sensitive to this scaling factor for any internal timeout that need to be used. HTH, -Joe On 4/3/2018 7:48 AM, Roger Riggs wrote: Hi Hamlin, Instead of a simple time delay, it would be useful to wait for the RegistryVM to terminate. In killRegistry: 149, adding subreg.waitFor() should be sufficient. 58: If using a 'for' loop it would be easier to understand if it included the usual start, increment and termination. Instead of burying it in the exception handler. 59, 102, 104: the introduction of the kill boolean makes the test harder to understand and seems to be unnecessary. the killRegistry() method already will only kill the subprocess if it still is alive. Roger On 4/2/2018 6:33 AM, Hamlin Li wrote: would you please review the following patch? bug: https://bugs.openjdk.java.net/browse/JDK-8188897 webrev: http://cr.openjdk.java.net/~mli/8188897/webrev.00/ Thank you -Hamlin
Re: RFR of JDK-8188897: java/rmi/registry/reexport/Reexport.java failed with Port already in use
Hello, Some general comments on the coding for tests like this: * It is preferable to avoid sleep in tests to avoid increasing the minimum amount of time a test takes to run. This helps limit the overall time it takes the test suite to run. * If timeouts are used, it is recommend to factor the maximum time waited with the jtreg timeout scaling factor; I don't recall its exact name. In other words, many of our tests are run on heavily loaded systems and the tests take longer than run than on typical laptops and workstations so jtreg is invoked with an timeout scaling factor. Individual tests should be sensitive to this scaling factor for any internal timeout that need to be used. HTH, -Joe On 4/3/2018 7:48 AM, Roger Riggs wrote: Hi Hamlin, Instead of a simple time delay, it would be useful to wait for the RegistryVM to terminate. In killRegistry: 149, adding subreg.waitFor() should be sufficient. 58: If using a 'for' loop it would be easier to understand if it included the usual start, increment and termination. Instead of burying it in the exception handler. 59, 102, 104: the introduction of the kill boolean makes the test harder to understand and seems to be unnecessary. the killRegistry() method already will only kill the subprocess if it still is alive. Roger On 4/2/2018 6:33 AM, Hamlin Li wrote: would you please review the following patch? bug: https://bugs.openjdk.java.net/browse/JDK-8188897 webrev: http://cr.openjdk.java.net/~mli/8188897/webrev.00/ Thank you -Hamlin
Re: RFR of JDK-8188897: java/rmi/registry/reexport/Reexport.java failed with Port already in use
Hi Hamlin, Instead of a simple time delay, it would be useful to wait for the RegistryVM to terminate. In killRegistry: 149, adding subreg.waitFor() should be sufficient. 58: If using a 'for' loop it would be easier to understand if it included the usual start, increment and termination. Instead of burying it in the exception handler. 59, 102, 104: the introduction of the kill boolean makes the test harder to understand and seems to be unnecessary. the killRegistry() method already will only kill the subprocess if it still is alive. Roger On 4/2/2018 6:33 AM, Hamlin Li wrote: would you please review the following patch? bug: https://bugs.openjdk.java.net/browse/JDK-8188897 webrev: http://cr.openjdk.java.net/~mli/8188897/webrev.00/ Thank you -Hamlin
