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

Exploring Sans-io #349

Open
ChihChengLiang opened this issue Nov 11, 2019 · 3 comments
Open

Exploring Sans-io #349

ChihChengLiang opened this issue Nov 11, 2019 · 3 comments

Comments

@ChihChengLiang
Copy link
Contributor

This is a different approach to fix #174

We first refactor classes using the Sans-io pattern. Separate out all async functions and then support both trio and even asyncio if we want to.

Here's an example how we can refactor the class GossipSub

before: The async function publish writes some data to peers

class GossipSub:
    async def publish(self, msg_forwarder: ID, pubsub_msg: rpc_pb2.Message) -> None:
        ...
        for peer_id in peers_gen:
            stream = self.pubsub.peers[peer_id]
            await stream.write(encode_varint_prefixed(rpc_msg.SerializeToString()))

after: maximally abstracting out the async part

class GossipSub:
   def publish(self, msg_forwarder: ID, pubsub_msg: rpc_pb2.Message) -> PublishEvent:
        ...
        return [PublishEvent(peer_id=peer_id, data=data) for peer_id in peers_gen]
            ...
            await stream.write(encode_varint_prefixed(rpc_msg.SerializeToString()))

class TrioGossipSubService:
    async def publish(self):
        events = self.gossipsub.publish(...)
        for e in events:
            stream = get_stream(e.peer_id)
            await stream.write(e.data)
  • Pros:
    • Clean separation of IO
    • Better testability
  • Cons:
    • Might change the interface
    • Takes more effort to refactor
    • The refactor might obscure the code logic
@ralexstokes
Copy link
Contributor

you could go a step further and put the calls to get_stream and stream.write into a(n async) closure that is returned from the publish function.

we then get a TrioHost or something that is basically just awaiting things and handling continuations between tasks

@ralexstokes
Copy link
Contributor

i like all of these pros :)

i agree that this is the more heavy-handed approach in that it is a more serious rewrite of the library, rather than just s/asyncio/trio.

@ShadowJonathan
Copy link
Contributor

This requires large parts of the library to be synchronous, which could take away advantages in trio which use await keywords as "checkpoints" to allow other code to run, making logic code actually asynchronous.

With this implimentation, logic code could run blocking indefinitely till it gives control back to its async-envoked entry point, after which only then async code is allowed to run (such as vital parts like ping, heartbeats, or connection management)

Unless these synchronous functions are ran in an alternative thread, and all calls and accesses to the other thread with the event loop are thread-safe and without race conditions, I personally don't think this is a good idea.

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

No branches or pull requests

3 participants