mojo: Add a runloop utility in python

mojo: Add a runloop utility in python

This is necessary to be able to use async operation on handles.

R=sdefresne@chromium.org,cmasone@chromium.org

Committed: https://crrev.com/830c88cca3c89dfabcbfc360ac7c035a020b8307
Cr-Commit-Position: refs/heads/master@{#295267}

[sdefresne, 2014-09-16 16:35:59]

https://codereview.chromium.org/552783004/diff/1/mojo/public/python/src/pytho...
File mojo/public/python/src/python_system_helper.cc (right):

https://codereview.chromium.org/552783004/diff/1/mojo/public/python/src/pytho...
mojo/public/python/src/python_system_helper.cc:16: PythonClosure(PyObject*
callable) {
If callable is NULL, then callable_ will be uninitialized leading to undefined
behavior in the destructor. Please initialize it or DCHECK that callable is not
NULL.

https://codereview.chromium.org/552783004/diff/1/mojo/public/python/src/pytho...
mojo/public/python/src/python_system_helper.cc:29: PyObject* empty_tuple =
PyTuple_New(0);
IIRC, Python function with a prototype returning a PyObject* only return NULL
when a Python exception was raised. I think you need to deal with the exception
here as I don't think the mojo base function know how to deal with a Python
exception asynchronously.

In addition to that, if I understand the code correctly, this function may be
called from another thread than the one that created the PythonClosure, right?
If so, I think this function (and maybe the destructor too if it can be called
from C++) must acquire the GIL before calling CPython code.

  virtual void Run() const MOJO_OVERRIDE {
    PyGILState_STATE gstate;
    gstate = PyGILState_Ensure();
    PyObject* result = RunWithGlobalInterpreterLock();
    if (result == NULL) {
      // Deal with Python exception
    } else {
      Py_DECREF(result);
    }
    PyGILState_Release(gstate);
  }

  PyObject* RunWithGlobalInterpreterLock() {
    PyObject* empty_tuple = PyTuple_New(0);
    if (!empty_tuple)
      return NULL;

    // ...
  }

See https://docs.python.org/2/c-api/init.html#non-python-created-threads

[qsr, 2014-09-17 11:15:48]

https://chromiumcodereview.appspot.com/552783004/diff/1/mojo/public/python/sr...
File mojo/public/python/src/python_system_helper.cc (right):

https://chromiumcodereview.appspot.com/552783004/diff/1/mojo/public/python/sr...
mojo/public/python/src/python_system_helper.cc:16: PythonClosure(PyObject*
callable) {
On 2014/09/16 16:35:59, sdefresne wrote:
> If callable is NULL, then callable_ will be uninitialized leading to undefined
> behavior in the destructor. Please initialize it or DCHECK that callable is
not
> NULL.

Done.

https://chromiumcodereview.appspot.com/552783004/diff/1/mojo/public/python/sr...
mojo/public/python/src/python_system_helper.cc:29: PyObject* empty_tuple =
PyTuple_New(0);
On 2014/09/16 16:35:59, sdefresne wrote:
> IIRC, Python function with a prototype returning a PyObject* only return NULL
> when a Python exception was raised. I think you need to deal with the
exception
> here as I don't think the mojo base function know how to deal with a Python
> exception asynchronously.

 Changed to deal with the exception in the following way: Quit the runloop, and
makes the 2 Run function for the runloop sending exception -> it means that if
you throw on a task, the run function will throw your exception. See the added
test case.

> In addition to that, if I understand the code correctly, this function may be
> called from another thread than the one that created the PythonClosure, right?

 Nope, that's not possible. But the thread will have release the GIL when this
method is called, so your point is still valid. Added a scoepd object to acquire
the GIL, and used it when needed.

> If so, I think this function (and maybe the destructor too if it can be called
> from C++) must acquire the GIL before calling CPython code.
> 
>   virtual void Run() const MOJO_OVERRIDE {
>     PyGILState_STATE gstate;
>     gstate = PyGILState_Ensure();
>     PyObject* result = RunWithGlobalInterpreterLock();
>     if (result == NULL) {
>       // Deal with Python exception
>     } else {
>       Py_DECREF(result);
>     }
>     PyGILState_Release(gstate);
>   }
> 
>   PyObject* RunWithGlobalInterpreterLock() {
>     PyObject* empty_tuple = PyTuple_New(0);
>     if (!empty_tuple)
>       return NULL;
> 
>     // ...
>   }
> 
> See https://docs.python.org/2/c-api/init.html#non-python-created-threads

[sdefresne, 2014-09-17 11:54:58]

lgtm

[qsr, 2014-09-17 12:33:11]

The CQ bit was checked by qsr@chromium.org

[commit-bot: I haz the power, 2014-09-17 12:33:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/552783004/40001

[commit-bot: I haz the power, 2014-09-17 13:28:10]

Committed patchset #3 (id:40001) as ecbec16aa1307343aec5f6928a7fc4b378e2e4da

[commit-bot: I haz the power, 2014-09-17 13:28:46]

Patchset 3 (id:??) landed as
https://crrev.com/830c88cca3c89dfabcbfc360ac7c035a020b8307
Cr-Commit-Position: refs/heads/master@{#295267}

[battre, 2014-09-17 15:42:25]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/575893004/ by battre@chromium.org.

The reason for reverting is: This fails on the build bots.

======================================================================
ERROR: testRunLoop (runloop_unittest.RunLoopTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File
"/mnt/data/b/build/slave/Linux_Tests/build/src/mojo/python/tests/runloop_unittest.py",
line 24, in testRunLoop
    loop = system.RunLoop()
AttributeError: 'module' object has no attribute 'RunLoop'

http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests/builds/1366....

[battre, 2014-09-17 15:45:50]

battre@chromium.org changed reviewers:
+ battre@chromium.org

[battre, 2014-09-17 15:45:51]

Could you please add a BUG= line to commit messages in the future? This way, I
can provide more feedback in case of reverts to the bug tracker that should not
all go into the revert message. Thanks.

Dominic