Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shutdown #937

Merged
merged 2 commits into from
May 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions jpype/_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,11 @@ def decorate(func):


def isJVMStarted():
""" True if the JVM is currently running."""
# TODO This method is horribly named. It should be named isJVMRunning as
# isJVMStarted would seem to imply that the JVM was started at some
# point without regard to whether it has been shutdown.
#
return _jpype.isStarted()


Expand Down Expand Up @@ -327,17 +332,25 @@ def shutdownJVM():
restart the JVM after being terminated.
"""
import threading
import jpype.config
if threading.current_thread() is not threading.main_thread():
raise RuntimeError("Shutdown must be called from main thread")
_jpype.shutdown()
if _jpype.isStarted():
_jpype.JPypeContext.freeResources = jpype.config.free_resources
_jpype.shutdown(jpype.config.destroy_jvm, False)


# In order to shutdown cleanly we need the reference queue stopped
# otherwise, we can experience a crash if a Java thread is waiting
# for the GIL.
def _JTerminate():
try:
_jpype.shutdown()
import jpype.config
# We are exiting anyway so no need to free resources
if _jpype.isStarted():
_jpype.JPypeContext.freeResources = False
if jpype.config.onexit:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be possible to cover this in the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

_jpype.shutdown(jpype.config.destroy_jvm, False)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@Thrameos Thrameos Mar 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

except RuntimeError:
pass

Expand Down
40 changes: 40 additions & 0 deletions jpype/config.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# *****************************************************************************
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
# See NOTICE file for details.
#
# *****************************************************************************

# This files holds hints used to control behavior of the JVM
# It is global variables rather than options to a function so that
# they can be set a module at any time rather than requiring arguments
# for a function.
#
# These options are to be treated as hints which may or may not be supported
# in future versions. Setting an unsupported option shall have no effect.
# Variables shall not be removed even if deprecated so that conditions
# based on these will be valid in all future versions.
#

onexit = True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"""If this is False, the JVM not will be notified of Python exit. Java will not execute any
resource cleanup routines."""

destroy_jvm = True
""" If this is False, the JVM will not execute any cleanup routines and memory will
not be freed."""

free_resources = True
""" If this is False, the resources will be allowed to leak after the shutdown call.
"""
2 changes: 1 addition & 1 deletion native/common/include/jp_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ class JPContext
bool ignoreUnrecognized, bool convertStrings, bool interrupt);
void attachJVM(JNIEnv* env);
void initializeResources(JNIEnv* env, bool interrupt);
void shutdownJVM();
void shutdownJVM(bool destroyJVM, bool freeJVM);
void attachCurrentThread();
void attachCurrentThreadAsDaemon();
bool isThreadAttached();
Expand Down
17 changes: 11 additions & 6 deletions native/common/jp_context.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ void JPContext::onShutdown()
m_Running = false;
}

void JPContext::shutdownJVM()
void JPContext::shutdownJVM(bool destroyJVM, bool freeJVM)
{
JP_TRACE_IN("JPContext::shutdown");
if (m_JavaVM == NULL)
Expand All @@ -383,12 +383,21 @@ void JPContext::shutdownJVM()
// JP_RAISE(PyExc_RuntimeError, "Cannot shutdown from embedded Python");

// Wait for all non-demon threads to terminate
JP_TRACE("Destroy JVM");
if (destroyJVM)
{
JP_TRACE("Destroy JVM");
JPPyCallRelease call;
m_JavaVM->DestroyJavaVM();
}

// unload the jvm library
if (freeJVM)
{
JP_TRACE("Unload JVM");
m_JavaVM = NULL;
JPPlatformAdapter::getAdapter()->unloadLibrary();
}

JP_TRACE("Delete resources");
for (std::list<JPResource*>::iterator iter = m_Resources.begin();
iter != m_Resources.end(); ++iter)
Expand All @@ -397,10 +406,6 @@ void JPContext::shutdownJVM()
}
m_Resources.clear();

// unload the jvm library
JP_TRACE("Unload JVM");
m_JavaVM = NULL;
JPPlatformAdapter::getAdapter()->unloadLibrary();
JP_TRACE_OUT;
}

Expand Down
30 changes: 17 additions & 13 deletions native/java/org/jpype/JPypeContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ public class JPypeContext
private final AtomicInteger shutdownFlag = new AtomicInteger();
private final List<Thread> shutdownHooks = new ArrayList<>();
private final List<Runnable> postHooks = new ArrayList<>();
public static boolean freeResources = true;

static public JPypeContext getInstance()
{
Expand Down Expand Up @@ -241,20 +242,23 @@ private void shutdown()
{
}

// Release all Python references
try
{
JPypeReferenceQueue.getInstance().stop();
} catch (Throwable th)
{
}

// Release any C++ resources
try
{
this.typeManager.shutdown();
} catch (Throwable th)
if (freeResources)
{
// Release all Python references
try
{
JPypeReferenceQueue.getInstance().stop();
} catch (Throwable th)
{
}

// Release any C++ resources
try
{
this.typeManager.shutdown();
} catch (Throwable th)
{
}
}

// Execute post hooks
Expand Down
21 changes: 16 additions & 5 deletions native/python/pyjp_module.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -277,10 +277,16 @@ static PyObject* PyJPModule_startup(PyObject* module, PyObject* pyargs)
JP_PY_CATCH(NULL);
}

static PyObject* PyJPModule_shutdown(PyObject* obj)
static PyObject* PyJPModule_shutdown(PyObject* obj, PyObject* pyargs, PyObject* kwargs)
{
JP_PY_TRY("PyJPModule_shutdown");
JPContext_global->shutdownJVM();
char destroyJVM = true;
char freeJVM = true;

if (!PyArg_ParseTuple(pyargs, "bb", &destroyJVM, &freeJVM))
return NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


JPContext_global->shutdownJVM(destroyJVM, freeJVM);
Py_RETURN_NONE;
JP_PY_CATCH(NULL);
}
Expand Down Expand Up @@ -560,7 +566,7 @@ static PyObject* PyJPModule_isPackage(PyObject *module, PyObject *pkg)
}


#if 0
#if 1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// GCOVR_EXCL_START
// This code was used in testing the Java slot memory layout. It serves no purpose outside of debugging that issue.
PyObject* examine(PyObject *module, PyObject *other)
Expand All @@ -574,12 +580,14 @@ PyObject* examine(PyObject *module, PyObject *other)
type = Py_TYPE(other);

printf("======\n");
int offset = 0;
if (!PyType_Check(other))
{
offset = PyJPValue_getJavaSlotOffset(other);
printf(" Object:\n");
printf(" size: %d\n", (int) Py_SIZE(other));
printf(" dictoffset: %d\n", (int) ((long long) _PyObject_GetDictPtr(other)-(long long) other));
printf(" javaoffset: %d\n", (int) PyJPValue_getJavaSlotOffset(other));
printf(" javaoffset: %d\n", offset);
}
printf(" Type: %p\n", type);
printf(" name: %s\n", type->tp_name);
Expand All @@ -597,6 +605,8 @@ PyObject* examine(PyObject *module, PyObject *other)
printf(" alloc: %p\n", type->tp_alloc);
printf(" free: %p\n", type->tp_free);
printf(" finalize: %p\n", type->tp_finalize);
long v = _PyObject_VAR_SIZE(type, 1)+(PyJPValue_hasJavaSlot(type)?sizeof (JPValue):0);
printf(" size?: %ld\n",v);
printf("======\n");

return PyBool_FromLong(ret);
Expand Down Expand Up @@ -656,7 +666,7 @@ static PyMethodDef moduleMethods[] = {
#else
{"startup", (PyCFunction) PyJPModule_startup, METH_VARARGS, ""},
// {"attach", (PyCFunction) (&PyJPModule_attach), METH_VARARGS, ""},
{"shutdown", (PyCFunction) PyJPModule_shutdown, METH_NOARGS, ""},
{"shutdown", (PyCFunction) PyJPModule_shutdown, METH_VARARGS, ""},
#endif
{"_getClass", (PyCFunction) PyJPModule_getClass, METH_O, ""},
{"_hasClass", (PyCFunction) PyJPModule_hasClass, METH_O, ""},
Expand All @@ -682,6 +692,7 @@ static PyMethodDef moduleMethods[] = {
#ifdef JP_INSTRUMENTATION
{"fault", (PyCFunction) PyJPModule_fault, METH_O, ""},
#endif
{"examine", (PyCFunction) examine, METH_O, ""},

// sentinel
{NULL}
Expand Down