Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calling publisher coroutine from another thread #3564

Closed
dwsutherland opened this issue Apr 10, 2020 · 4 comments · Fixed by #4274
Closed

Calling publisher coroutine from another thread #3564

dwsutherland opened this issue Apr 10, 2020 · 4 comments · Fixed by #4274
Assignees
Labels
bug Something is wrong :( efficiency For notable efficiency improvements
Milestone

Comments

@dwsutherland
Copy link
Member

dwsutherland commented Apr 10, 2020

While reviewing #3492 I noticed the publisher was being called differently.
I had a closer look after re-basing a branch and realised the publisher is running in a different thread, and awaiting the publish coroutine from the main thread:

await self.publisher.publish(
self.data_store_mgr.get_publish_deltas())

This isn't desirable (although may not be an issue):
https://docs.python.org/3/library/asyncio-dev.html#concurrency-and-multithreading

So, to be safe, we have two options here:

  1. Use a threadsafe call to this coroutine from the main thread (i.e. loop.call_soon_threadsafe()).
  2. As the publisher.py has no blocking calls or loops (unlike the REQ/RES server.py), we could run the publisher in the main thread and keep the awaited publish the same.

In my opinion, and assuming publishing is not that intensive/blocking; we should go with option 2, as it simplifies the workflow scheduler somewhat (if extra thread isn't needed, why use it).

Thoughts? (ping @oliver-sanders)

Pull requests welcome!

@dwsutherland dwsutherland added the bug Something is wrong :( label Apr 10, 2020
@dwsutherland dwsutherland added this to the cylc-8.0a2 milestone Apr 10, 2020
@dwsutherland dwsutherland added the efficiency For notable efficiency improvements label Apr 10, 2020
@oliver-sanders
Copy link
Member

The publisher doesn't need to be in lock-step, call_soon_threadsafe should be enough?

@hjoliver hjoliver modified the milestones: cylc-8.0a2, cylc-8.0a3 Apr 30, 2020
@oliver-sanders
Copy link
Member

oliver-sanders commented Jul 2, 2020

After working on #3654 I'm keen that we stop spawning/managing threads in the add-hoc way we currently do (I've hit all sorts of lovely issues).

As of #3654 the Scheduler is "event loop agnostic", it gets executed in a single loop provided by the executor.

How about re-working the server/publisher code to be async and, if possible, agnostic to the async implementation (plus or minus a couple of asyncio.gather calls). Then just use future = loop.run_in_executor(...) to start the server and await future to stop it. This would cut out the need for Barrier synchronisation etc.

@dwsutherland
Copy link
Member Author

How about re-working the server/publisher code to be async and, if possible, agnostic to the async implementation (plus or minus a couple of asyncio.gather calls). Then just use future = loop.run_in_executor(...) to start the server and await future to stop it. This would cut out the need for Barrier synchronisation etc.

Yeah, we can do this, but we needed to change the socket.recv loop to the no-wait version otherwise it will cause the whole program to hang.

@dwsutherland
Copy link
Member Author

dwsutherland commented Jul 2, 2020

And actually, I think the publisher is already in a state that it can run in the same thread...
It was setup to run in the different thread because it used asyncio and the scheduler didn't (which isn't the case now), ergo option 2..

The ZMQ authenticator still runs in it's own thread (which is fine)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :( efficiency For notable efficiency improvements
Projects
None yet
4 participants