[subprocess][go] run the subprocesses from go-land #843
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
[py] install pympler too [py] fix multiple issues with python memory collector [py] initialize entries
|
Found another race condition on CircleCI (not locally) fixing that.... In this case the race only surfaces with golang 1.8.3 (and I presume others), but not with our default Data race on No trace of the problem on Related material: |
|
It looks like there's still a GIL related bug, it could also be a "sticky" thread issue - will address tomorrow. |
|
It may be fixed now after locking the go routine to the thread. Haven't experienced the weird crashes for a while now but I will need to verify tomorrow. |
|
Feel free to merge now to test this a bit with a packaged agent, I went through the code quickly and it looks good. I'll make comments from a thorough review on this PR, a bit later :) |
olivielpeau
reviewed
Nov 24, 2017
| Py_RETURN_NONE; | ||
| } | ||
|
|
||
| if (!PyList_Check(cmd_args)) { |
There was a problem hiding this comment.
for subprocess.Popen, cmd_args could also be a sequence (i.e. not strictly a list) or a string. We don't have to handle this here (we can handle the conversion in the python get_subprocess_output), but we should if we want to preserve the api.
| if (!PyArg_ParseTuple(args, "O|O:get_subprocess_output", &cmd_args, &cmd_raise_on_empty)) { | ||
| PyGILState_Release(gstate); | ||
| PyErr_SetString(PyExc_TypeError, "unable to parse arguments"); | ||
| Py_RETURN_NONE; |
There was a problem hiding this comment.
We should use return NULL instead of Py_RETURN_NONE when we set an error on the interpreter (otherwise, in python land, the function returns None and doesn't raise an exception).
| if (!PyList_Check(cmd_args)) { | ||
| PyGILState_Release(gstate); | ||
| PyErr_SetString(PyExc_TypeError, "command args not a list"); | ||
| Py_RETURN_NONE; |
| if (cmd_raise_on_empty != NULL && !PyBool_Check(cmd_raise_on_empty)) { | ||
| PyGILState_Release(gstate); | ||
| PyErr_SetString(PyExc_TypeError, "bad raise on empty argument - should be bool"); | ||
| Py_RETURN_NONE; |
| if(!(subprocess_args = (char **)malloc(sizeof(char *)*subprocess_args_sz))) { | ||
| PyGILState_Release(gstate); | ||
| PyErr_SetString(PyExc_MemoryError, "unable to allocate memory, bailing out"); | ||
| Py_RETURN_NONE; |
| cErr := C.CString("unable to import subprocess empty output exception") | ||
| C.PyErr_SetString(C.PyExc_Exception, cErr) | ||
| C.free(unsafe.Pointer(cErr)) | ||
| return C._none() |
| cErr := C.CString("get_subprocess_output expected output but had none.") | ||
| C.PyErr_SetString((*C.PyObject)(unsafe.Pointer(excClass)), cErr) | ||
| C.free(unsafe.Pointer(cErr)) | ||
| return C._none() |
| C.free(unsafe.Pointer(cModuleName)) | ||
| if utilModule == nil { | ||
| cErr := C.CString("unable to import subprocess empty output exception") | ||
| C.PyErr_SetString(C.PyExc_Exception, cErr) |
There was a problem hiding this comment.
not sure we should overwrite the original exception
| C.free(unsafe.Pointer(cExcName)) | ||
| if excClass == nil { | ||
| cErr := C.CString("unable to import subprocess empty output exception") | ||
| C.PyErr_SetString(C.PyExc_Exception, cErr) |
There was a problem hiding this comment.
not sure we should overwrite the original exception
| @@ -75,6 +75,67 @@ func TestFindSubclassOf(t *testing.T) { | |||
| assert.Equal(t, 1, sclass.RichCompareBool(barClass, python.Py_EQ)) | |||
| } | |||
|
|
|||
| func TestSubprocessBindings(t *testing.T) { | |||
There was a problem hiding this comment.
could you also write a test that executes a python check that uses get_subprocess_output? (a bit like what the tests in check_test.go do)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Runs the subprocess from go-land as opposed to forking from within the python interpreter. This should hopefully address memory leak issues we have experienced, as well as resolve the zombie fork issue (triggered by an obscure race-condition) we have experienced.
Motivation
The
subprocess32approach in #830 has helped with the memory leak, but wasn't a truly satisfactory fix as it didn't quite fix the issue on windows. The alternative, setting a global lock on windows would likely be less desirable than a pure-go cross-platform fix. Hopefully this addresses that.Additional Notes
I've been unable to get the tests to fly on gitlab, they seem fine locally. Let's see what happens on CircleCI. Let's be thorough with the GIL, reference count during the review 😅