RFR 8239893: Windows handle Leak when starting processes using ProcessBuilder
Please review a change to the Windows ProcessImpl to ensure that the handles created for the input and output streams of a process are closed when no longer referenced. Unlike on Linux, there is no thread monitoring the process that can close the streams. The FileDescriptors are registered with the Cleaner to be closed when they are no longer referenced. A test is added that monitors the count of handles as 50 Processes are launched and exit. The test and change only affect the Windows implementation. Webrev: http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893/ Issue: https://bugs.openjdk.java.net/browse/JDK-8239893 Thanks, Roger
Hi Roger,
On Mar 5, 2020, at 12:51 PM, Roger Riggs <Roger.Riggs@oracle.com> wrote:
Please review a change to the Windows ProcessImpl to ensure that the handles created for the input and output streams of a process are closed when no longer referenced.
Unlike on Linux, there is no thread monitoring the process that can close the streams. The FileDescriptors are registered with the Cleaner to be closed when they are no longer referenced.
A test is added that monitors the count of handles as 50 Processes are launched and exit. The test and change only affect the Windows implementation.
Webrev: http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893/ <http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893/>
Issue: https://bugs.openjdk.java.net/browse/JDK-8239893 <https://bugs.openjdk.java.net/browse/JDK-8239893> In CheckHandles.java at line 72 there is this calculation: final long ERROR_THRESHOLD = minHandles + (minHandles / ERROR_PERCENT); // 10% increase over min to passing max Do you think it would be better as
final long ERROR_THRESHOLD = minHandles + ((minHandles + ERROR_PERCENT - 1) / ERROR_PERCENT); // 10% increase over min to passing max , i.e., rounded instead of truncated? Brian
Hi Brian, Updated webrev: http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893-1/index.html The threshold is a pretty loose target. Given the original error that leaked a handle for every process started, it just needs to detect a growing number of handles in use. But for the sake of accuracy and avoiding someone copying buggy code, its fixed. Thanks, Roger On 3/5/20 4:07 PM, Brian Burkhalter wrote:
Hi Roger,
On Mar 5, 2020, at 12:51 PM, Roger Riggs <Roger.Riggs@oracle.com <mailto:Roger.Riggs@oracle.com>> wrote:
Please review a change to the Windows ProcessImpl to ensure that the handles created for the input and output streams of a process are closed when no longer referenced.
Unlike on Linux, there is no thread monitoring the process that can close the streams. The FileDescriptors are registered with the Cleaner to be closed when they are no longer referenced.
A test is added that monitors the count of handles as 50 Processes are launched and exit. The test and change only affect the Windows implementation.
Webrev: http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893/
In CheckHandles.java at line 72 there is this calculation: final long ERROR_THRESHOLD = minHandles + (minHandles / ERROR_PERCENT); // 10% increase over min to passing max Do you think it would be better as
final long ERROR_THRESHOLD = minHandles + ((minHandles + ERROR_PERCENT - 1) / ERROR_PERCENT); // 10% increase over min to passing max
, i.e., rounded instead of truncated?
Brian
Hi Roger, Looks good! Brian
On Mar 6, 2020, at 8:16 AM, Roger Riggs <Roger.Riggs@oracle.com> wrote:
Updated webrev: http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893-1/index.html <http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893-1/index.html>
The threshold is a pretty loose target. Given the original error that leaked a handle for every process started, it just needs to detect a growing number of handles in use. But for the sake of accuracy and avoiding someone copying buggy code, its fixed.
Hi Roger, In the test, the error value (-1) from the native code seems silently suppressed. Should it be caught/reported in the java side? nit: copyright year in ProcessImpl.java -> 2020. Naoto On 3/5/20 12:51 PM, Roger Riggs wrote:
Please review a change to the Windows ProcessImpl to ensure that the handles created for the input and output streams of a process are closed when no longer referenced.
Unlike on Linux, there is no thread monitoring the process that can close the streams. The FileDescriptors are registered with the Cleaner to be closed when they are no longer referenced.
A test is added that monitors the count of handles as 50 Processes are launched and exit. The test and change only affect the Windows implementation.
Webrev: http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893/
Issue: https://bugs.openjdk.java.net/browse/JDK-8239893
Thanks, Roger
Hi Naoto, Updated webrev: http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893-1/index.html I don't think getting handle count of your own process can ever fail. But in the abundance of caution, added a check and failure. Copyright updated. Thanks, Roger Corrected copyright On 3/5/20 4:14 PM, naoto.sato@oracle.com wrote:
Hi Roger,
In the test, the error value (-1) from the native code seems silently suppressed. Should it be caught/reported in the java side?
nit: copyright year in ProcessImpl.java -> 2020.
Naoto
On 3/5/20 12:51 PM, Roger Riggs wrote:
Please review a change to the Windows ProcessImpl to ensure that the handles created for the input and output streams of a process are closed when no longer referenced.
Unlike on Linux, there is no thread monitoring the process that can close the streams. The FileDescriptors are registered with the Cleaner to be closed when they are no longer referenced.
A test is added that monitors the count of handles as 50 Processes are launched and exit. The test and change only affect the Windows implementation.
Webrev: http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893/
Issue: https://bugs.openjdk.java.net/browse/JDK-8239893
Thanks, Roger
Looks good. Thanks for the update. Naoto On 3/6/20 8:17 AM, Roger Riggs wrote:
Hi Naoto,
Updated webrev: http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893-1/index.html
I don't think getting handle count of your own process can ever fail. But in the abundance of caution, added a check and failure.
Copyright updated.
Thanks, Roger
Corrected copyright
On 3/5/20 4:14 PM, naoto.sato@oracle.com wrote:
Hi Roger,
In the test, the error value (-1) from the native code seems silently suppressed. Should it be caught/reported in the java side?
nit: copyright year in ProcessImpl.java -> 2020.
Naoto
On 3/5/20 12:51 PM, Roger Riggs wrote:
Please review a change to the Windows ProcessImpl to ensure that the handles created for the input and output streams of a process are closed when no longer referenced.
Unlike on Linux, there is no thread monitoring the process that can close the streams. The FileDescriptors are registered with the Cleaner to be closed when they are no longer referenced.
A test is added that monitors the count of handles as 50 Processes are launched and exit. The test and change only affect the Windows implementation.
Webrev: http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893/
Issue: https://bugs.openjdk.java.net/browse/JDK-8239893
Thanks, Roger
participants (3)
-
Brian Burkhalter
-
naoto.sato＠oracle.com
-
Roger Riggs