async client #50

chadawagner · 2023-08-08T06:38:03Z

Adds support for async clients

This PR addresses #43 and #46

chadawagner · 2023-08-12T00:31:19Z

@ofpiyush Would you be open to including an async client such as this? Happy to discuss if you have other ideas about how I should go about it. Thanks!

chadawagner · 2023-08-12T00:32:37Z

twirp/base.py

@@ -101,6 +101,6 @@ def _get_encoder_decoder(self, endpoint, headers):
        else:
            raise exceptions.TwirpServerException(
                code=errors.Errors.BadRoute,
-                message="unexpected Content-Type: " + ctype
+                message="unexpected Content-Type: " + str(ctype)


ofpiyush · 2023-08-12T15:31:18Z

@chadawagner nice work!

In principle, I love the idea of Async client and from design POV, I don't see any issues.

Having said that, I haven't looked at Python code or kept up with Twirp in a good while, not enough to do a proper review.

Tagging @thecreator232 and @avinassh to take a look.

chadawagner · 2023-08-29T00:41:59Z

@chadawagner nice work!

In principle, I love the idea of Async client and from design POV, I don't see any issues.

Having said that, I haven't looked at Python code or kept up with Twirp in a good while, not enough to do a proper review.

Tagging @thecreator232 and @avinassh to take a look.

Hi @ofpiyush, @thecreator232, @avinassh just following up, any chance someone has some time for a review soon?

avinassh · 2023-08-29T04:41:17Z

The PR looks great to me! but I don't have write/merge access anymore.

Tagging @thecreator232 @raghav39

chadawagner · 2023-08-29T15:55:42Z

The PR looks great to me! but I don't have write/merge access anymore.

Tagging @thecreator232 @raghav39

Thanks @avinassh!

MiLk · 2023-09-13T03:13:09Z

twirp/async_client.py

+    def __init__(self, address, timeout=5, session=None):
+        self._address = address
+        self._timeout = timeout
+        self._session = session
+        self._should_close_session = False
+
+    def __del__(self):
+        if self._should_close_session:
+            try:
+                loop = asyncio.get_event_loop()
+                if loop.is_running():
+                    loop.create_task(self._session.close())
+                elif not loop.is_closed():
+                    loop.run_until_complete(self._session.close())
+            except RuntimeError:
+                pass
+
+    @property
+    def session(self):
+        if self._session is None:
+            self._session = aiohttp.ClientSession(
+                self._address, timeout=aiohttp.ClientTimeout(total=self._timeout)
+            )
+            self._should_close_session = True
+        return self._session


The go implementation doesn't create a client automatically.

I think it should be the same here.

Suggested change

def __init__(self, address, timeout=5, session=None):

self._address = address

self._timeout = timeout

self._session = session

self._should_close_session = False

def __del__(self):

if self._should_close_session:

try:

loop = asyncio.get_event_loop()

if loop.is_running():

loop.create_task(self._session.close())

elif not loop.is_closed():

loop.run_until_complete(self._session.close())

except RuntimeError:

pass

@property

def session(self):

if self._session is None:

self._session = aiohttp.ClientSession(

self._address, timeout=aiohttp.ClientTimeout(total=self._timeout)

)

self._should_close_session = True

return self._session

def __init__(self, address: str, session: ClientSession) -> None:

self._address = address

self._session = session

async def aclose(self) -> None:

await self._session.close()

It can be used like this:

http_client_options = { "timeout": aiohttp.ClientTimeout(total=timeout_s) } async with aiohttp.ClientSession(**http_client_options) as session: client = AsyncHaberdasherClient(address, session) await client.MakeHat(...) ... async with contextlib.aclosing(AsyncHaberdasherClient( address, aiohttp.ClientSession(**http_client_options) )) as client: await client.MakeHat(...) ...

Great! Yes, I agree, and I didn't love the idea of creating the session automatically, but I wasn't sure if some would prefer a simpler way to use it without having to provide and manage the underlying session. (Because the existing sync implementation with requests has no such session requirement, I was hesitant to add one, and was trying to keep the usage similar for those who wanted to keep it that way.)

However:

aiohttp.ClientSession can only be created within a coroutine, so if you are required to provide a session when you init the twirp client then you can only do so within a coroutine. (This is not how we currently init our client dependencies within our app, so I had planned to pass the client session with each request, but that might feel cumbersome to some folks)

If the caller creates and manages the client session, I'd think they should probably handle the closing of it also, rather than having the twirp client close the session

Thoughts?

I think you can solve your problem by turning the client into an async context manager.
It would be something like that:

def init(): client = AsyncHaberdasherClient(address, session=None) ... async def use(client: AsyncHaberdasherClient): async with ( aiohttp.ClientSession() as session, client.with_session(session), ): await client.MakeHat(...)

with_session can be implemented like this:

@asynccontextmanager async def with_session(session: ClientSession): backup, self._session = self._session, session yield backup, self._session = None, backup

Or using a factory? (need testing)

def init(): async def session_factory(): return aiohttp.ClientSession() client = AsyncHaberdasherClient(address, session_factory=session_factory) ... async def use(client: AsyncHaberdasherClient): async with client: await client.MakeHat(...)

You would need to implement __aenter__ and __aexit__ on AsyncTwirpClient to call the functions on the session.

After thinking a bit more about this, maybe you could just pass the http_client_options instead of a factory to the constructor when creating the twirp client.

Then you can use __aenter__ and __aexit__ to create a temporary session which can be reused for several calls.
But it could also create a session directly inside _make_request on each call for simplicity when there is no need to reuse a client session.

Thanks for the suggestions @MiLk! I don't mind passing the session in each request, but do you think I should add with_session or session_factory as something that's generally useful for others? I pushed some changes removing the auto-created client, and would be fine merging as-is but would like to cover other common use cases.

I didn't see that the session could be passed directly with each request.
You can disregard my previous suggestions.
I think that's enough, and there is no need to make the code more complex.

chadawagner · 2023-09-20T01:58:55Z

@avinassh I pushed new changes to remove the auto-created client session (the caller is now required to provide a session either on init or per request). Just letting you know in case you'd like to take another look, I'll assume your approval still stands unless I hear otherwise 😄

chadawagner · 2023-09-21T15:26:09Z

@ofpiyush Looks like we have some consensus and a couple of approvals here, any chance we could merge and cut a 0.0.8 release soonish? Please lmk if there's any way I can help with that. Thanks!

ofpiyush · 2023-09-22T02:51:25Z

Both @avinassh and I have moved on from the company and don't have write access anymore.

Tagging @thecreator232 and @raghav39 to possibly check with internal services and make a release.

chadawagner · 2023-10-04T17:34:48Z

@thecreator232 @raghav39 any hope for a merge/release here? This PR was opened 2 months ago 😭

chadawagner · 2023-10-21T00:18:00Z

@thecreator232 @raghav39 @avinassh @ofpiyush we seem to be at a standstill here. Is there any hope of getting a PR merge and release? Or is this repo officially abandoned?

ravipetlur · 2023-10-22T08:35:30Z

Let me get this reviewed this week for the current services dependent and we will have this actioned.

chadawagner · 2023-11-06T15:27:50Z

Hi @ravipetlur, any luck?

ravipetlur · 2023-11-06T18:26:47Z

@chadawagner We are getting this checked internally, let me get this expedited. Sorry for the delay.

WarrierRajeev

LGTM. I tested the async client with internal services as well. Didn't spot any problems. We can merge this 👍🏻
The only issue is when running setup.py, it expects a version.txt. PEP440 doesn't allow for a version like 0.0.8 so maybe we can add a file there with a version number 0.1.0 or something?

chadawagner · 2023-11-16T19:20:14Z

LGTM. I tested the async client with internal services as well. Didn't spot any problems. We can merge this 👍🏻 The only issue is when running setup.py, it expects a version.txt. PEP440 doesn't allow for a version like 0.0.8 so maybe we can add a file there with a version number 0.1.0 or something?

Hi @WarrierRajeev ! Yeah I'm 👍 to increasing the version, is that something y'all can handle internally? Idk what your release pipeline looks like. In addition to whatever setup.py wants, there's some Pipfile and requirements.txt stuff in the example dir that probably need updating.

chadawagner · 2023-12-04T04:38:31Z

Hi again @WarrierRajeev @ravipetlur any updates on a merge and release? 😬

chadawagner added 7 commits August 7, 2023 13:35

add async client

bcb9560

create/close single aiohttp session

53c38c9

update template

e648d9e

update example

6d270a3

fix bug in missing content-type exception

f54d2fd

session param in init, better teardown and examples

814dff4

formatting

744006e

chadawagner commented Aug 12, 2023

View reviewed changes

chadawagner mentioned this pull request Aug 12, 2023

Async Client support #43

Open

avinassh approved these changes Aug 29, 2023

View reviewed changes

MiLk reviewed Sep 13, 2023

View reviewed changes

chadawagner added 2 commits September 18, 2023 18:08

don't auto-create client session

15daba4

update example

183e5ff

MiLk approved these changes Sep 19, 2023

View reviewed changes

WarrierRajeev approved these changes Nov 8, 2023

View reviewed changes

ravipetlur merged commit 69b39fb into verloop:main Dec 12, 2023

chadawagner deleted the cw/async-client branch March 6, 2024 12:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

async client #50

async client #50

chadawagner commented Aug 8, 2023 •

edited

Loading

chadawagner commented Aug 12, 2023

chadawagner Aug 12, 2023

ofpiyush commented Aug 12, 2023

chadawagner commented Aug 29, 2023

avinassh commented Aug 29, 2023

chadawagner commented Aug 29, 2023

MiLk Sep 13, 2023

chadawagner Sep 19, 2023

MiLk Sep 19, 2023

MiLk Sep 19, 2023

chadawagner Sep 19, 2023

MiLk Sep 19, 2023

chadawagner commented Sep 20, 2023

chadawagner commented Sep 21, 2023

ofpiyush commented Sep 22, 2023

chadawagner commented Oct 4, 2023

chadawagner commented Oct 21, 2023

ravipetlur commented Oct 22, 2023

chadawagner commented Nov 6, 2023

ravipetlur commented Nov 6, 2023

WarrierRajeev left a comment

chadawagner commented Nov 16, 2023

chadawagner commented Dec 4, 2023

async client #50

async client #50

Conversation

chadawagner commented Aug 8, 2023 • edited Loading

chadawagner commented Aug 12, 2023

chadawagner Aug 12, 2023

Choose a reason for hiding this comment

ofpiyush commented Aug 12, 2023

chadawagner commented Aug 29, 2023

avinassh commented Aug 29, 2023

chadawagner commented Aug 29, 2023

MiLk Sep 13, 2023

Choose a reason for hiding this comment

chadawagner Sep 19, 2023

Choose a reason for hiding this comment

MiLk Sep 19, 2023

Choose a reason for hiding this comment

MiLk Sep 19, 2023

Choose a reason for hiding this comment

chadawagner Sep 19, 2023

Choose a reason for hiding this comment

MiLk Sep 19, 2023

Choose a reason for hiding this comment

chadawagner commented Sep 20, 2023

chadawagner commented Sep 21, 2023

ofpiyush commented Sep 22, 2023

chadawagner commented Oct 4, 2023

chadawagner commented Oct 21, 2023

ravipetlur commented Oct 22, 2023

chadawagner commented Nov 6, 2023

ravipetlur commented Nov 6, 2023

WarrierRajeev left a comment

Choose a reason for hiding this comment

chadawagner commented Nov 16, 2023

chadawagner commented Dec 4, 2023

chadawagner commented Aug 8, 2023 •

edited

Loading