search

mosquito / aiohttp-xmlrpc

XMLRPC for aiohttp
MIT License
34 stars 19 forks source link

Change close to be a coroutine #35

Closed cooperlees closed 4 years ago

cooperlees commented 4 years ago

Addresses parts of #27

cooperlees commented 4 years ago

Would you prefer me to have the function return aiohttp's coroutine?

I mainly care here cause I have another PR ready to go to add aenter and aexit for async gernerator support. This will help me clean up this code:

https://github.com/pypa/bandersnatch/blob/master/src/bandersnatch/master.py#L145

(I'd also like to get the USER_AGENT easier too so I can modify it - But that will come after this :))

mosquito commented 4 years ago

I hope my PRs (#493 and #492) will describe options how it might be fixed.

Of course, it not should be merged, that's only just to describe my vision.

cooperlees commented 4 years ago

As stated on the PRs, I'd really like to reduce the amount of code I need in bandersnatch to use your library. I feel this will benefit many other users and future users. My end goal is adding aenter and aexit to your class so I can just async with in bandersnatch and significantly reduce the lines of code.

A second goal is to also make it easier to change the USER_AGENT header for queries. We do this so metrics generated can be used to easily identify Mirroring traffic as opposed to CI / real humans.

Also, I'd love to at least see the integration tests in bandersnatch's CI pass in order to be confident your proposals actually work.

cooperlees commented 4 years ago

Would you accept a separate async def aclose() coroutine/method that I can then use with the __aenter__ and __aexit__ I'd like to add in a follow up PR?

mosquito commented 4 years ago

To closing the session you should use an original close (from master branch) prefixed with await keyword.

cooperlees commented 4 years ago

Ok, so I've tried as your wished in this commit in the close_xml_rpc branch:

As you can see CI passes, but when you add PYTHONFAULTHANDLER=yes and PYTHONWARNINGS=once you see the connections are not actually closed. To illustrate this I set these on to bandersnatch's Travis 3.8 Integration run:

This CI job reports that the connections are not getting closed. Here is a paste of the errors: https://pastebin.com/J9Fm2mnc

If I revert these changes and keep PYTHONFAULTHANDLER=yes + PYTHONWARNINGS=once, the connections are no longer reported as unclosed.

This is why I submitted the PR. How do you want to fix this and allow people to actually close connections?

cooperlees commented 4 years ago

Found https://github.com/aio-libs/aiohttp/issues/111

Moved to more correctly using my shared ClientSession. Don't need this PR.