added goroutines example

[esendjer]

What does this PR do?

Adding an example of using goroutines

Motivation

The issue #4

Additional Notes

This example is up to date and tested

[christian-korneck]

@esendjer thanks for working on this! Much appreciated.

Could you please review your Go code and make sure that:

• where PyObjects are returned from calling Python C-API functions, there's proper Python memory handling in place? (i.e. .DecRef() objects where needed)
• where Python C-API functions are called there's Python error handling in place (i.e. check python3.PyErr_Occurred() if needed)

For example for fooModule := python3.PyImport_ImportModule(nameModule):

• If you look at the docs for PyImport_ImportModule it says "Return value: New reference.". So you need something like defer fooModule.Decref() after error checking. Otherwise you might have a memory leak. Decref only after error checking, otherwise the result is undefined.
• regarding errors the doc for this function says: " Return a new reference to the imported module, or NULL with an exception set on failure.". So it might make sense to check for both. You can check for the exception with python3.PyErr_Occurred() (and print error messages with python3.PyErr_Print(), etc).

(If you need more explanation on the memory handling, I wrote a blogpost here).

Let me know when done and I'll continue to review. Thanks!

[esendjer]

@christian-korneck thanks for your points and advice.
I've tried to improve the PR according to your comment and a bit more and hope it was successful. Could you please have a look at this?

Thanks!

[christian-korneck]

	fooModule := python3.PyImport_ImportModule(nameModule)
	defer fooModule.DecRef()
	if python3.PyErr_Occurred() != nil {

a PyObject shouldn't get decref'ed before you know for sure that you actually got a PyObject that needs decref'ing.
(see Example)

[esendjer]

Got it. Thanks. I'll fix this.

[esendjer]

In the last commit, I decided to stop using defer somePyObject.DecRef() because sometimes it caused randomly panic on my tests on a call Py_Finalize(). It happens not for each running but it's like 50/50 - sometimes there is panic, sometimes there is not.

If I understood well it happens because so is the logic of working Py_FinalizeEx/Py_Finalize. According to the article https://docs.python.org/3/c-api/init.html#c.Py_FinalizeEx: The destruction of modules and objects in modules is done in random order.
I'm not sure but maybe panic happens when Py_Finalize try to destroy an object which was removed by a defer statement. but one more time - I'm not sure, not sure at all that my suggestion is correct or near to be true.

Instead applying defer somePyObject.DecRef() I moved a call DecRef() for python objects under if statements, where checks error, such as:

nameModule := "foo"
	fooModule := python3.PyImport_ImportModule(nameModule)
	if fooModule == nil && python3.PyErr_Occurred() != nil {
		err = errors.New("Error importing the python module")
		fooModule.DecRef()
		return
	}

Sorry for lots of attention and some mistakes, I'm just a beginner 😔

[christian-korneck]

Sorry for lots of attention and some mistakes, I'm just a beginner 😔

@esendjer no worries, that's okay.

I'm not sure, not sure at all that my suggestion is correct

The main challenge of this task is to get everything "correct" and being relatively certain about it. This requires understanding of how memory management with the CPython C-API works, amongst other things (GIL, etc).

If you want to move on with this task (I highly encourage you to do so, there's lots to learn), I would suggest that you take the time and familiarize yourself with this topic area. For example you could start by using the blogpost/tutorial that I've mentioned above as a first tutorial, then write a few programs, etc and make sure you understand them and are able to debug them. You will likely need to consult other docs, etc as well (docs and examples are usually for C but translate well into Go).

If you have any specific questions feel free to ask. (Also stackoverflow, etc are good places if you stick to C, if that's an option for you).

For this PR: I'll continue to review if/when you're confident that your code is "correct". Also just to say, there's no rush. I don't think anyone else is working on this. Take your time. (However, I could also understand if this doesn't work for you - feel free to close the PR in that case).

[esendjer]

Thanks a ton for your explanation and your article, and sorry for missing some important points from it.

As I see now, my issue was in that I tried to decrement the reference count for the limit after that the reference of it had already been stolen by PyTuple_SetItem, and I had to do nothing with the limit there because a new owner is caring about it, and when I tried to call DecRef for the limit directly the result became undefined and it caused to panic.

I've fixed this issue, turned back using defer for decrementing reference counts, and this time did it in the right way. Now, this example works well for all my test cases.

[christian-korneck]

Thanks. Before I review it, could you go over it, check for the usual things and improve the code if needed?
(Keep in mind that this is an example others will be blindly trusting and building their own code on top. It doesn't need to be perfect, but also shouldn't promote any 'no gos').

• correctness: python error handling and memory management
• correctness: GIL locking
• is the code as idiomatic as possible?
• does it lint?
• would it make sense to have tests? (don't think it's a must have here, but you decide)
• check for other relevant things mentioned here (not all of these are relevant for such a simple "cli tool")

[esendjer]

Thank you. I'll continue working on this point-by-point. And thanks a lot for the useful links.

I'm a bit confused about the 2nd point - correctness: GIL locking. I'm sure that working with the GIL in the right way and there aren't gaps in acquiring or releasing the GIL in the code.

There are a few places where the code works with the GIL:

	// Initialize the Python interpreter and
	// since version 3.7 it also creates the GIL explicitly by calling PyEval_InitThreads()
	// so you don’t have to call PyEval_InitThreads() yourself anymore
	python3.Py_Initialize() // <-- create the GIL, the GIL is locked by the main thread
	
	// ... Following is importing of Python module
	// and declaration of variables, getting attributes
	// and others action of creating/defining of PyObject

	// Save the current state and release the GIL
	// so that goroutines can acquire it
	state := python3.PyEval_SaveThread() // <-- release the GIL, the GIL is unlocked for using by goroutines
	
	// the 1st goroutine
	go func() {
		_gstate := python3.PyGILState_Ensure() // <-- acquire the GIL, the GIL is locked by the 1st goroutine
		// ... call python code
		python3.PyGILState_Release(_gstate) // <-- release the GIL, the GIL is unlocked for using by others
		// ... 
	}()

	// the 2nd goroutine
	go func() {
		_gstate := python3.PyGILState_Ensure() // <-- acquire the GIL, the GIL is locked by the 2nd goroutine
		// ... call python code
		python3.PyGILState_Release(_gstate) // <-- release the GIL, the GIL is unlocked for using by others
		// ... 
	}()

	// Restore the state and lock the GIL
	python3.PyEval_RestoreThread(state) // <-- acquire the GIL, the GIL is locked by the main thread

Here I follow the next pattern of embedding Python code:

1. Save the state and lock the GIL.
2. Do Python.
3. Restore the state and unlock the GIL.

And for each locking here is releasing, in the end, the lock is returned to the main thread

Unfortunately, I have no idea how yet I can be sure more of the correctness of the GIL locking/unlocking. I think I can add some checks with PyGILState_Check() before doing python, but in this case, it looks unnecessarily. If you have some advice about this I'll thankful for sharing them.

[christian-korneck]

confused about the 2nd point - correctness: GIL locking.

this was just meant in a very generic way. The code must not have bugs or flaws in that aspect (as it's supposed to be an example for it). I haven't really looked at your code yet and it's not meant as critic of something specific.

[esendjer]

Thanks! Now, this point is clear to me.

[esendjer]

I went through the checklist and tried to cover so many points as possible.

• ✔️ correctness: python error handling and memory management
• ✔️ correctness: correctness: GIL locking
• ✔️ is the code as idiomatic as possible?
• ✔️ does it lint?
  The output of linters:

  ===[ Linter #0 - go vet ]===
  + go vet ./...
  Exit code: 0
  ===[ Linter #1 - go fmt ]===
  + go fmt ./...
  Exit code: 0
  ===[ Linter #2 - golint ]===
  + golint ./...
  Exit code: 0
  ===[ Linter #3 - errcheck ]===
  + errcheck ./...
  Exit code: 0
  ===[ Linter #4 - staticcheck ]===
  + staticcheck ./...
  Exit code: 0
  

• ✅ would it make sense to have tests? (don't think it's a must have here, but you decide)
  There is only one main function here, that doesn't take any parameters or args from stdin, and I'm not sure that it makes sense to do some tests.
• ✅ check for other relevant things mentioned here (not all of these are relevant for such a simple "cli tool")
  Some not quite well things here are, such as errors.New(…) instead modelling them, but I believe this is ok for the example.

[christian-korneck]

Thanks for this. Just as a quick heads up: I'm currently a bit slow to respond, but will look at this PR soon. Thanks for your patience!

[liam-lian]

there are also some problems when the go-routine is scheduled to another thread，see this page: https://studygolang.com/articles/12822.
I think you should add runtime.LockOSThread() for each go-routine in your example !!!
thx。

[christian-korneck]

@esendjer I'm getting closer to being able to review this (partly because of #9 ). Could you please add runtime.LockOSThread() or describe why you didn't add it?

[jshiwam]

hi team,

I have a program where I want to DecRef a PyObject(the return value of python methods) in a go routine. The code inside go routine looks something like this:

(... redacted by moderator and moved to github discussions: #16 (comment))

[christian-korneck]

@jshiwam and everyone - just to do some housekeeping: I don't think this PR discussion is the best place for your comment. Why? Well it's a question about a specific code problem that you're having and not really concret feedback or suggestions about the PR that @esendjer is crafting.

We're relatively liberal about allowing usage questions as issues here, even if they have little or nothing to do with this project (which is just a thin Go wrapper around the CPython C-API), but are often really CPython C-API usage questions. But issues and PRs should still stay on topic.

As we're seeing usage questions from time to time, I have now enabled Github Discussions, with a "Usage Discussion" section:
https://github.com/go-python/cpy3/discussions

I hope this allows us to:

• focus issues to bug, enhancement, question discussions specific to this project (i.e. bugs in this Go library, not the CPython C-API)
• allow any kind of broad discussion how to achieve specific things using this library alongside the CPython C-API in Github Discussions

@jshiwam That's why I've redacted your above comment and moved your full question to Github Discussions and posted a first reply to it:
#16 (comment)

[christian-korneck]

docs for PyTuple_New don't mention a Python error gets raised on failure, so this should probably only be a check for NULL?
(When an error gets raised on failure usually the docs say something like Return <something good>, or NULL with an exception set on failure. But not here).

[jshiwam]

@christian-korneck @esendjer if no one is working on this PR then I can take this up and finish it, as these examples are really important for people who use this wrapper lib.

[christian-korneck]

@jshiwam sure, all input is welcome!

[jshiwam]

Hi @christian-korneck sorry for the delayed response, had been busy recently. I have made the final changes in this code. According to my understanding only using runtime.LockOSThread() had to be called inside the goroutines. Also would I have to create a new PR? because the changes are in my fork.

[jshiwam]

Here please check it out.
https://github.com/go-python/cpy3/pull/21/files