Added example for go routines #21
Conversation
There was a problem hiding this comment.
@jshiwam thanks for this. I've left some comments from my first read over the code (likely incomplete, but should get you an idea of the kinds of things that might need improvement?).
| defer python3.Py_Finalize() | ||
|
|
||
| // Prints any python error if it was here | ||
| // no needs to call it after each single check of PyErr_Occurred() |
There was a problem hiding this comment.
I don't think that's safe. The docs say:
Call this function only when the error indicator is set. Otherwise it will cause a fatal error!
examples/goroutines/main.go
Outdated
| defer odds.DecRef() | ||
|
|
||
| even := fooModule.GetAttrString("print_even") // new reference, a call DecRef() is needed | ||
| if even == nil && python3.PyErr_Occurred() != nil { |
There was a problem hiding this comment.
I'm not sure if a failed GetAttrString will cause a PyErr. At least the docs don't mention it. Maybe only check for nil return value (but not PyErr_Occured) ?
https://docs.python.org/3/c-api/object.html#c.PyObject_GetAttrString
There was a problem hiding this comment.
@christian-korneck failed GetAttrString will cause a PyErr. Here is how I Verified it. I added some prints to check on what happens when the GetAttrString fails as follows:
odds := fooModule.GetAttrString("print_odds") // new reference, a call DecRef() is needed
if odds == nil && python3.PyErr_Occurred() != nil {
err = errors.New("error getting the attribute print_odds")
fmt.Println(python3.PyUnicode_AsUTF8(python3.PyErr_Occurred().Repr()), odds)
python3.PyErr_Print()
return
}
defer odds.DecRef()
Here is the output:
<class 'AttributeError'> <nil>
AttributeError: module 'foo' has no attribute 'print_odds'
2022/08/16 10:29:00 error getting the attribute print_odds
There was a problem hiding this comment.
I've asked in Python IRC what the cannonical way to do error handling is and was pointed to this:
"You normally don’t need to call PyErr_Occurred() to see whether an error occurred in a function call, since you should be able to tell from the return value."
https://docs.python.org/3/extending/extending.html#intermezzo-errors-and-exceptions
Looking at large open source projects it seems like people are using a lot of different variants. But just checking for NULL seems to be sufficient?
(I still need to check a few things, but probably will update my examples).
|
|
||
| // Cleans the Go variable, because now a new owner is caring about related PyObject | ||
| // no action, such as a call DecRef(), is needed here | ||
| limit = nil |
There was a problem hiding this comment.
this cleanup is too early? In the next block you're still using limit (to decref in case PyTuple_SetItem has failed).
examples/goroutines/main.go
Outdated
| } | ||
|
|
||
| args := python3.PyTuple_New(1) // new reference, a call DecRef() is needed | ||
| if args == nil && python3.PyErr_Occurred() != nil { |
There was a problem hiding this comment.
again the docs aren't clear if this causes a PyErr. Only check for nil?
There was a problem hiding this comment.
Even I am not sure on this one, for GetAttrString I was able to check if PyErr_Occured() gets triggered or not, but not sure how to check that here, for now will just check for nil.
| } | ||
| defer even.DecRef() | ||
|
|
||
| limit := python3.PyLong_FromGoInt(50) // new reference, will stolen later, a call DecRef() is NOT needed |
There was a problem hiding this comment.
again the docs aren't clear if this causes a PyErr. Only check for nil?
examples/goroutines/main.go
Outdated
|
|
||
| // At the end of all, if there was an error | ||
| // prints the error and exit with the non-zero code | ||
| defer func() { |
There was a problem hiding this comment.
do we need to do this error check with a defer? Wouldn't it maybe simpler and more idiomatic to wrap most part of main() in a new function that returns an error? And then call it from my main and check for an error there?
examples/goroutines/main.go
Outdated
| } | ||
|
|
||
| var wg sync.WaitGroup | ||
| wg.Add(2) |
There was a problem hiding this comment.
would move this down right before the Goroutines start, for better readability, etc.
examples/goroutines/main.go
Outdated
| // so that goroutines can acquire it | ||
| state := python3.PyEval_SaveThread() // release the GIL, the GIL is unlocked for using by goroutines | ||
|
|
||
| go func() { |
There was a problem hiding this comment.
would it be better to pass args as argument to the anonymous func? not sure
examples/goroutines/main.go
Outdated
| go func() { | ||
| runtime.LockOSThread() | ||
| _gstate := python3.PyGILState_Ensure() // acquire the GIL, the GIL is locked by the 2nd goroutine | ||
| even.Call(args, python3.PyDict_New()) |
There was a problem hiding this comment.
I think this call will return a PyObject (of type None, new reference), that you need to decref.
https://stackoverflow.com/a/8451069
https://docs.python.org/3/c-api/none.html#c.Py_None
|
@christian-korneck let me know if this PR needs some other changes |
|
sorry for the delay, I'm currently slow to respond. I'll have a look soon. |
This PR contains example for goroutines