-
Notifications
You must be signed in to change notification settings - Fork 186
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
Shutdown #937
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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() | ||
|
||
|
||
|
@@ -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: | ||
_jpype.shutdown(jpype.config.destroy_jvm, False) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't understood why we wouldn't want to use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
||
|
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
""" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
|
@@ -560,7 +566,7 @@ static PyObject* PyJPModule_isPackage(PyObject *module, PyObject *pkg) | |
} | ||
|
||
|
||
#if 0 | ||
#if 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
@@ -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); | ||
|
@@ -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); | ||
|
@@ -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, ""}, | ||
|
@@ -682,6 +692,7 @@ static PyMethodDef moduleMethods[] = { | |
#ifdef JP_INSTRUMENTATION | ||
{"fault", (PyCFunction) PyJPModule_fault, METH_O, ""}, | ||
#endif | ||
{"examine", (PyCFunction) examine, METH_O, ""}, | ||
|
||
// sentinel | ||
{NULL} | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?