Skip to content
This repository has been archived by the owner on Dec 10, 2018. It is now read-only.

Fully Asyncio Support #308

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from
Open

Fully Asyncio Support #308

wants to merge 20 commits into from

Conversation

ethe
Copy link
Member

@ethe ethe commented Sep 3, 2017

I find #246 this issue so I wrote an asyncio support for thriftpy, and it can pass all the original test cases. Now you can use thriftpy like this:

######
# This is a thrift client
######
import thriftpy
import asyncio
from thriftpy.rpc import make_aio_client


echo_thrift = thriftpy.load("echo.thrift", module_name="echo_thrift")


async def request():
    client = await make_aio_client(
        echo_thrift.EchoService, '127.0.0.1', 6000)
    print(await client.echo('hello, world'))
    client.close()
######
# This is a thrift server
######
import asyncio
import thriftpy

from thriftpy.rpc import make_aio_server

echo_thrift = thriftpy.load("echo.thrift", module_name="echo_thrift")


class Dispatcher(object):
    async def echo(self, param):
        print(param)
        await asyncio.sleep(0.1)
        return param


def main():
    server = make_aio_server(
        echo_thrift.EchoService, Dispatcher(), '127.0.0.1', 6000)
    server.serve()


if __name__ == '__main__':
    main()

@wooparadog wooparadog requested a review from hit9 September 4, 2017 02:56
@ethe ethe changed the title Fully Asyncio support Fully Asyncio Support Sep 4, 2017
Copy link
Member

@wooparadog wooparadog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too familiar with how to write an asyncio library. But this pr tries to solve the same problem as #299. But #299 has a much smaller changeset.

Could you please provide more information?

@@ -0,0 +1,441 @@
# -*- coding: utf-8 -*-
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file contains too many copied code. At least some of them can be referenced instead of copied

Copy link
Member Author

@ethe ethe Sep 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wooparadog The asyncio support of #299 based on TFramedTransport transport which is easier to be implemented. This transport tries to read the whole message out from socket at the begin and save it into buffer, it is easier because it just needs yield once from reading the whole message. My implementation based on another transport (TBinaryTransport) so it may be a little bit verbose.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's unavoidable for simpler abstraction, there should be a better code path like:

  • contrib/aio/binary_transport
  • contrib/aio/framed_transport

but it hurts so much for all those duplicate code...

@asyncio.coroutine
def _send(self, _api, **kwargs):
self._oprot.write_message_begin(_api, TMessageType.CALL, self._seqid)
args = getattr(self._service, _api + "_args")()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thriftpy simply ignored seqid, but we better fix it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's not a problem about asyncio support, maybe we can create another pull request to solve it both sync and async module.

@ethe
Copy link
Member Author

ethe commented Jul 20, 2018

Yeah I think it can be merged smoothly, I just resolved the last little conflict.

@ethe
Copy link
Member Author

ethe commented Jul 20, 2018

@wooparadog The different between two pull requests is they realized different transport (I support bufferd and #299 supports memory bufferd). In my opinion, the buffered transport realization is more difficult than framed transport on this feature. And I also find that there are some blocking I/O at the low level not be yielded out in #299 realization (read_frame, etc.), cause it still used blocking socket, and this pull request is fully non-blocking (based on non-blocking stream reader and writer).

@HIRANO-Satoshi
Copy link

@hit9 Could you merge this PR?

@hit9
Copy link
Contributor

hit9 commented Aug 7, 2018

Sorry but I am removed from this organization.

1533624849161

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants