capnproto / pycapnp

Cap'n Proto serialization/RPC system - Python bindings
BSD 2-Clause "Simplified" License
458 stars 125 forks source link

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

Closed LasseBlaauwbroek closed 9 months ago

LasseBlaauwbroek commented 1 year ago

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 commented 1 year ago

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

haata commented 1 year ago

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

LasseBlaauwbroek commented 1 year ago

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 commented 1 year ago

Just wanted to express that we are interested in this change. We are testing MR https://github.com/capnproto/pycapnp/pull/317 to see if it solves some issues we encountered when another Python library exists that also links to capnp