`loadPackagesFromImports` in `PyodideConsole.runcode` isn't locked

[CNSeniorious000]

🐛 Bug

Console.runcode is locked, so it acts like a normal synchronous python console. The only difference PyodideConsole.runcode made is calling loadPackagesFromImports before running super().runcode.

But loadPackagesFromImports is not locked, so it may cause confusing behaviors:

To Reproduce

from pyodide.console import PyodideConsole
c = PyodideConsole()

c.push("import numpy as np")
c.push("np")

Expected behavior

<ConsoleFuture pending>
<ConsoleFuture finished exception=NameError("name 'np' is not defined")>

Environment

• Pyodide Version: 0.26.1
• Browser: Edge
• Any other relevant information:

Additional context

Console.runcode is locked:

https://github.com/pyodide/pyodide/blob/9f49030ef511296574885720282887bea6c6fd54/src/py/pyodide/console.py#L384-L394

While PyodideConsole.runcode is not:

https://github.com/pyodide/pyodide/blob/9f49030ef511296574885720282887bea6c6fd54/src/py/pyodide/console.py#L492-L507

[ryanking13]

Thanks for the report @CNSeniorious000! I guess then we should lock the total block including loadPackgesFromImports?

I think splitting Console.runcode such as:

class Console:
  def runcode(...):
      with self._acquire_lock():
          self._run_code()

then changing PyodideConsole.runcode accordingly:

class PyodideConsole(Console):
  def runcode(...):
    with self._acquire_lock():
        await loadPackagesFromImports()
        super()._runcode()

would do the job.

[CNSeniorious000]

Yeah this is a good approach. But I think writing with self._acquire_lock() twice may be not elegant enough.

Since runcode is a standalone method, and usually the whole method is locked, why not implement a _runcode (or runcode_inner I am not sure about the naming) that is just returns runcode with a lock? Like this:

class Console:
    async def runcode(...):
        ...  ＃ don't use the lock
    async def _runcode(...):
        async with self._lock:
            return await self.runcode(...)
    def runsource(...):
        ...
        return ensure_future(self._runcode(...))

class PyodideConsole(Console):
    async def runcode(...):
        ...  ＃ don't need to change

That is, lock behavior is moved into a _runcode method, instead of inside runcode. This way will make Console easier to inherit.

---

Another approach I was thinking is that using decorators may make the code cleaner:

Since self._lock isn't used elsewhere, we can embed the locking logic into a decorator, like this:

def locked(func):
    lock = Lock()
    async def wrapper(*args, **kwargs):
        async with lock:
            return await func(*args, **kwargs)
    return wrapper

I'm not sure about the naming too, maybe it should be with_lock or something else.

Then use it to decorate any method needs to be locked. Like runcode. I think the outcome is thatthis way the code may be cleaner, while the downside is that the PyodideConsole.runcode will be locked twice, which is a bit waste.

So how do you think of these two approaches?

[ryanking13]

That is, lock behavior is moved into a _runcode method, instead of inside runcode. This way will make Console easier to inherit.

Sounds good to me. It would be nice if the naming of _runcode was a bit more explicit, so that it's clear from the name that it holds a lock.

Then use it to decorate any method needs to be locked. Like runcode.

For now, we don't have other places that use the lock, so I think we are okay with the current approach. I think with lock: is very straightforward, and adopting a decorator doesn't make it more clearer.