Shutdown #937

Thrameos · 2021-02-12T18:22:03Z

Test PR. Not clear it this deals with the issue.

codecov · 2021-02-12T18:30:21Z

Codecov Report

Merging #937 (d1f42b8) into master (505da82) will decrease coverage by 0.09%.
The diff coverage is 70.83%.

@@            Coverage Diff             @@
##           master     #937      +/-   ##
==========================================
- Coverage   87.76%   87.66%   -0.10%     
==========================================
  Files         112      114       +2     
  Lines       10011    10069      +58     
  Branches     4045     4048       +3     
==========================================
+ Hits         8786     8827      +41     
- Misses        622      634      +12     
- Partials      603      608       +5

Impacted Files	Coverage Δ
native/common/include/jp_context.h	`70.27% <ø> (ø)`
native/common/jp_context.cpp	`81.74% <20.00%> (-2.36%)`	⬇️
native/python/pyjp_module.cpp	`82.68% <66.66%> (-0.37%)`	⬇️
jpype/_core.py	`95.06% <88.88%> (-1.23%)`	⬇️
jpype/config.py	`100.00% <100.00%> (ø)`
native/common/jp_platform.cpp	`62.50% <0.00%> (-25.00%)`	⬇️
native/common/jp_method.cpp	`94.73% <0.00%> (-1.17%)`	⬇️
native/common/jp_methoddispatch.cpp	`80.95% <0.00%> (-0.96%)`	⬇️
jpype/_jinit.py	`100.00% <0.00%> (ø)`
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 505da82...d1f42b8. Read the comment docs.

lgtm-com · 2021-02-12T18:32:07Z

This pull request introduces 1 alert when merging 800a363 into 505da82 - view on LGTM.com

new alerts:

1 for Use of the return value of a procedure

Thrameos · 2021-02-12T19:13:34Z

@marscher Is the style of these config options acceptable?

marscher · 2021-02-15T10:58:07Z

jpype/_core.py

@@ -96,6 +96,10 @@ def decorate(func):


 def isJVMStarted():
+    """ This method is horribly named.  It should be named isJVMRunning as


This can be a comment (e.g. TODO), but not a docstring IMO. It is confusing for the user.

Any recommendations for a docstring as it doesnt do what its name implies? This has been bothering me for a while. We have three states being described by one bool flag: jvm not started, jvm running, jvm shutdown.

marscher · 2021-02-15T10:59:48Z

jpype/_core.py

+        # We are exiting anyway so no need to free resources
+        if _jpype.isStarted():
+            _jpype.JPypeContext.freeResources = False
+        if jpype.config.onexit:


would it be possible to cover this in the tests?

We can hit each path but I am unsure what to test for as these options are mostly about how much memory to free. Agressively clearing memory just to call exit is an invitation to trigger race conditions. But what behavior change should should be tested for here?

marscher · 2021-02-15T11:07:23Z

native/python/pyjp_module.cpp

@@ -560,7 +566,7 @@ static PyObject* PyJPModule_isPackage(PyObject *module, PyObject *pkg)
 }


-#if 0
+#if 1


Do you expect to use this debug code in the future? If not, I recommend getting rid of it. It is stored in the history of the file anyways.

This was a leak for another PR. This particular test code gets used whenever I debug memory issues with slots. It was reactivated for #933. Maybe i should just tag it as a branch as it is likely to get reused. Or perhaps on the instrumentation compile flag.

marscher · 2021-02-15T11:08:09Z

jpype/config.py

+# based on these will be valid in all future versions.
+#
+
+onexit = True


This style is plain and simple and could be replaced with something more complicated (if ever needed) in the future. So I completely agree with it.

Is the name config reasonable? Is there any other behaviors that should be configurable in this way besides shutdown?

The basic idea is if there were multiple modules using jpype then the config module could gather all the requirements. Most modules would use default so would never set these options. This avoids having a huge number of options to startJVM and requiring the main from having to gather up all module requirements explicitely.

marscher · 2021-02-15T11:08:41Z

native/python/pyjp_module.cpp

+	char freeJVM = true;
+
+	if (!PyArg_ParseTuple(pyargs, "bb", &destroyJVM, &freeJVM))
+		return NULL;


a subrun would be nice to cover this behavior as well.

marscher · 2021-02-15T11:09:29Z

I hope these potential leak checks on osx-py39 are false positives...

Thrameos · 2021-02-15T13:38:06Z

The mac leaks are false positives. There is nothing that requires memory to be cleaned up on requests for gc and unless the gc is a true stop the world the request may not be complete before we gather statistics. It is sort of a hint. If there was a leak it should steadly increase rather than failing to shrink a few times which is what triggers this check. We can reduce the false positives by running larger batches or waiting longer for the gc to complete. But both options cause the test bench to run more slowly.

Thrameos · 2021-02-19T17:12:50Z

@marscher it was pointed out in the JNI documentation that unloading the JVM is not supported. Should the default be free_jvm=False?

marscher · 2021-02-19T22:33:36Z

Yes. But whats the point of offering an option for an unsupported feature anyways?

Thrameos · 2021-02-19T23:31:33Z

We can remove the free_jvm option, but that has been the behavior of the JPype since v0.5.5.1 in the project so I was trying to preserve the default behavior unless I can definitively determine that it is the source of the crash. And with these transient failures I haven't found any way to prove definitely what is causing these issues.

From the bowels of the jpype git history jvm unloading was introduced in

commit d92b13d9a4f97f60895550de80159bb25c21ea40
Author: marscher 
Date:   Thu Feb 20 16:09:42 2014 +0100

    added methods to unload the jvm library (fixed a todo task)

I am not sure why we free the shared library in the first place other than an attempt to keep memory from leaking. Most codes do not attempt to free a loaded shared library because handlers added into memory would likely cause a segfault unless something specifically goes and uninstalls every connection.

If unloading the library is unnecessary (or unwise) I can certainly clean up some code by removing that from the platform handlers. But honestly I am not sure what the JNI documentation is trying to indicate.

In JNI version 7 it says....

Unloading the VM

The JNI_DestroyJavaVM() function unloads a Java VM. As of JDK/JRE 1.1, only the main thread could unload the VM, by calling DestroyJavaVM. As of JDK/JRE 1.2, the restriction was removed, and any thread may call DestroyJavaVM to unload the VM.

The VM waits until the current thread is the only non-daemon user thread before it actually unloads. User threads include both Java threads and attached native threads. This restriction exists because a Java thread or attached native thread may be holding system resources, such as locks, windows, and so on. The VM cannot automatically free these resources. By restricting the current thread to be the only running thread when the VM is unloaded, the burden of releasing system resources held by arbitrary threads is on the programmer.

(our problems mostly stem from the last statement, there is no way that I know to ensure that Python threads has deleted any contact to Java nor has a Java method on the call stack.)

And later it says...

DestroyJavaVM

jint DestroyJavaVM(JavaVM *vm);

Unloads a Java VM and reclaims its resources.
The support for DestroyJavaVM was not complete in JDK/JRE 1.1. As of JDK/JRE 1.1 Only the main thread may call DestroyJavaVM. Since JDK/JRE 1.2, any thread, whether attached or not, can call this function. If the current thread is attached, the VM waits until the current thread is the only non-daemon user-level Java thread. If the current thread is not attached, the VM attaches the current thread and then waits until the current thread is the only non-daemon user-level thread. The JDK/JRE still does not support VM unloading, however.

So does that last statement mean

The JVM is not unloaded by the destroy call (even though the previous section said it did).
The JVM shared library must be held even after the destroy (and failure to do so would be unsafe).
This is an outdated statement that no one has looked at since version Java 1.2
All of the above?

pelson · 2021-03-06T05:26:10Z

jpype/_core.py

+        if _jpype.isStarted():
+            _jpype.JPypeContext.freeResources = False
+        if jpype.config.onexit:
+            _jpype.shutdown(jpype.config.destroy_jvm, False)


I haven't understood why we wouldn't want to use jpype.config.free_jvm instead of the False literal here. Is this a mistake, or intentional?

There is no point in freeing if we are 90% of the way to exit. Is there?

Thrameos · 2021-05-07T16:25:37Z

I haven't the time to test all the options as a subrun mostly because it is hard to know what the tests should be. At most we can just run coverage tests to see if each fails.

This should go in 1.3

This was referenced Feb 12, 2021

Non-deterministic SIGSEGV with pytest + log4j #934

Open

SIGSEGV on termination #842

Open

JVM shutdown differences after 0.7.5 #936

Open

Add config for shutdown

5a58351

Thrameos force-pushed the shutdown branch from 800a363 to 5a58351 Compare February 12, 2021 18:40

marscher reviewed Feb 15, 2021

View reviewed changes

pelson reviewed Mar 6, 2021

View reviewed changes

john-hen mentioned this pull request May 4, 2021

Java InterruptedException at end of Python session MPh-py/MPh#38

Closed

Remove free_jvm, fix comment

d1f42b8

Thrameos merged commit 55261da into jpype-project:master May 7, 2021

Thrameos deleted the shutdown branch May 7, 2021 16:34

pelson mentioned this pull request Feb 5, 2024

Rare crashes on interpreter exit #1144

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shutdown #937

Shutdown #937

Thrameos commented Feb 12, 2021

codecov bot commented Feb 12, 2021 •

edited

Loading

lgtm-com bot commented Feb 12, 2021

Thrameos commented Feb 12, 2021

marscher Feb 15, 2021

Thrameos Feb 15, 2021

marscher Feb 15, 2021

Thrameos Feb 15, 2021

marscher Feb 15, 2021

Thrameos Feb 15, 2021

marscher Feb 15, 2021

Thrameos Feb 15, 2021

marscher Feb 15, 2021

marscher commented Feb 15, 2021

Thrameos commented Feb 15, 2021

Thrameos commented Feb 19, 2021

marscher commented Feb 19, 2021

Thrameos commented Feb 19, 2021

Unloading the VM

DestroyJavaVM

pelson Mar 6, 2021

Thrameos Mar 6, 2021 •

edited

Loading

Thrameos commented May 7, 2021

		@@ -96,6 +96,10 @@ def decorate(func):


		def isJVMStarted():
		""" This method is horribly named. It should be named isJVMRunning as

               }
-              #if 0
+              #if 1

Shutdown #937

Shutdown #937

Conversation

Thrameos commented Feb 12, 2021

codecov bot commented Feb 12, 2021 • edited Loading

Codecov Report

lgtm-com bot commented Feb 12, 2021

Thrameos commented Feb 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marscher commented Feb 15, 2021

Thrameos commented Feb 15, 2021

Thrameos commented Feb 19, 2021

marscher commented Feb 19, 2021

Thrameos commented Feb 19, 2021

Unloading the VM

DestroyJavaVM

Choose a reason for hiding this comment

Thrameos Mar 6, 2021 • edited Loading

Choose a reason for hiding this comment

Thrameos commented May 7, 2021

codecov bot commented Feb 12, 2021 •

edited

Loading

Thrameos Mar 6, 2021 •

edited

Loading