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

Proposal: Wrap the kj event loop in a context manager #316

Closed
LasseBlaauwbroek opened this issue Jun 9, 2023 · 4 comments · Fixed by #317
Closed

Proposal: Wrap the kj event loop in a context manager #316

LasseBlaauwbroek opened this issue Jun 9, 2023 · 4 comments · Fixed by #317

Comments

@LasseBlaauwbroek
Copy link
Contributor

Currently, pycapnp creates the kj event loop on-the-fly, whenever it is needed (erroring out when the asyncio event loop is not already running). A reference to the kj event loop is kept until the asyncio event loop is closed. However, if there are still pycapnp objects around that reference the kj event loop, it will keep running even though the asyncio loop is closed and no more events will ever be scheduled. Furthermore, if a new asyncio loop is started and the old kj event loop is still running, no new kj loop can be started (because only one kj loop per thread can exist).

All this leads to subtle bugs (it is easy to get segmentation faults) and complicated logic in pycapnp. I propose that instead of allocating the kj loop on-the-fly, we allocate it in a context-manager. This will require all users to write something like this:

def main():
    with capnp.kj_loop():
        <all your cool capnp code>

if __name__ == "__main__":
    asyncio.run(main())

Or possibly we can also include a function like this:

def main2():
    <all your cool capnp code>

def main():
    capnp.run(main2())

if __name__ == "__main__":
    asyncio.run(main())

And we could even combine asyncio.run and capnp.run:

def main():
    <all your cool capnp code>

if __name__ == "__main__":
    capnp.run_asyncio(main())

The purpose of the context-manager is to check that the kj event loop is no longer needed when we exit the context manager. If references to the event loop still exist during exit, we will throw an exception.

Thoughts @haata?

@haata
Copy link
Collaborator

haata commented Jun 9, 2023

Hmm, let me ponder this for a couple days. But I think this could work.

@haata
Copy link
Collaborator

haata commented Jun 12, 2023

I think I like it. I'm thinking with all these changes we should do a major version increment at this point.

@LasseBlaauwbroek
Copy link
Contributor Author

Yes, I thought we had already settled on a major version increment. I'm trying to get all breaking changes I have in mind in quickly for that purpose.

@fabiorossetto
Copy link

Just wanted to express that we are interested in this change. We are testing MR #317 to see if it solves some issues we encountered when another Python library exists that also links to capnp

@haata haata closed this as completed in #317 Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants