Merge pull request #253 from rstudio/session-iteration-error

Fix errors resulting from session close

Author: jcheng5

.github/workflows/pytest.yaml

@@ -75,9 +75,7 @@ jobs:
       - name: Run unit tests
         if: steps.install.outcome == 'success' && (success() || failure())
         run: |
-          pwd
-          cd tests
-          pytest
+          make test
 
       - name: Type check with pyright
         if: steps.install.outcome == 'success' && (success() || failure())

Makefile

@@ -58,6 +58,7 @@ lint: ## check style with flake8
 	flake8 --show-source --max-line-length=127 shiny tests examples
 
 test: ## run tests quickly with the default Python
+	python3 tests/asyncio_prevent.py
 	pytest
 
 test-all: ## run tests on every Python version with tox

shiny/_app.py

@@ -310,7 +310,9 @@ def _request_flush(self, session: Session) -> None:
     async def _flush_pending_sessions(self) -> None:
         # TODO: Until we have reactive domains, flush all sessions (because we
         # can't yet keep track of which ones need a flush)
-        for _, session in self._sessions.items():
+        # Use list() to create a copy, in case self._sessions mutates from under us,
+        # which would cause a "dictionary changed size during iteration" RuntimeError
+        for _, session in list(self._sessions.items()):
             await session._flush()
         # for id, session in self._sessions_needing_flush.items():
         #     await session.flush()

shiny/reactive/_core.py

@@ -119,9 +119,24 @@ def __init__(self) -> None:
         )
         self._next_id: int = 0
         self._pending_flush_queue: PriorityQueueFIFO[Context] = PriorityQueueFIFO()
-        self.lock = asyncio.Lock()
+        self._lock: Optional[asyncio.Lock] = None
         self._flushed_callbacks = _utils.AsyncCallbacks()
 
+    @property
+    def lock(self) -> asyncio.Lock:
+        """
+        Lock that protects this ReactiveEnvironment. It must be lazily created, because
+        at the time the module is loaded, there generally isn't a running asyncio loop
+        yet. This causes the asyncio.Lock to be created with a different loop than it
+        will be invoked from later; when that happens, acquire() will succeed if there's
+        no contention, but throw a "hey you're on the wrong loop" error if there is.
+        """
+        if self._lock is None:
+            # Ensure we have a loop; get_running_loop() throws an error if we don't
+            asyncio.get_running_loop()
+            self._lock = asyncio.Lock()
+        return self._lock
+
     def next_id(self) -> int:
         """Return the next available id"""
         id = self._next_id

tests/asyncio_prevent.py

@@ -0,0 +1,39 @@
+"""This script is NOT designed to be run as part of a pytest suite; it demands a clean
+Python process because it needs to be run before the shiny module is loaded.
+
+The purpose of this script is to ensure that no shiny module initialization code uses
+the event loop. The running event loop during shiny module startup is different than the
+running event loop during the app's operation (I assume uvicorn creates the latter). The
+creation of, say, an asyncio.Lock during the course of shiny module startup will result
+in race conditions if the lock is used during the app's operation."""
+
+if __name__ == "__main__":
+    import sys
+    import asyncio
+    import importlib
+    from typing import Optional
+
+    if "shiny" in sys.modules:
+        raise RuntimeError(
+            "Bad test: shiny was already loaded, it's important that SpyEventLoopPolicy"
+            " is installed before shiny loads"
+        )
+
+    class SpyEventLoopPolicy(asyncio.DefaultEventLoopPolicy):
+        def get_event_loop(self):
+            raise RuntimeError("get_event_loop called")
+
+        def set_event_loop(self, loop: Optional[asyncio.AbstractEventLoop]):
+            raise RuntimeError("set_event_loop called")
+
+        def new_event_loop(self):
+            raise RuntimeError("new_event_loop called")
+
+    asyncio.set_event_loop_policy(SpyEventLoopPolicy())
+
+    # Doing this instead of "import shiny" so no linter is tempted to remove it
+    importlib.import_module("shiny")
+    sys.stderr.write(
+        "Success; shiny module loading did not attempt to access an asyncio event "
+        "loop\n"
+    )