[HZ-5276] [HZ-5277] Asyncio Module Cloud Support#754
Conversation
- Fixed a bug where an asyncio Lock was tried to be acquired without a release - Store asyncio tasks in order not to lose them - Removed start/shutdown from AsyncioReactor
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #754 +/- ##
==========================================
- Coverage 94.73% 94.57% -0.16%
==========================================
Files 392 393 +1
Lines 24959 25074 +115
==========================================
+ Hits 23645 23714 +69
- Misses 1314 1360 +46 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| response = await self._authenticate(connection) | ||
| translated = await self._translate_member_address(member) | ||
| connection = self._create_connection(translated) | ||
| await connection._create_task |
There was a problem hiding this comment.
What's the reason not returning a "future" on self._create_connection but instead of doing it on connection._create_task ?
There was a problem hiding this comment.
_create_connection returns the AsyncioConnection object itself, since it is used in _authenticate.
_create_connection also creates two tasks, one is for creating the socket, and the second is for checking whether the connection was established in time.
We await on connection._create_task to make sure the socket is created before carrying on with authentication.
There was a problem hiding this comment.
Still, it looks odd to me. Instead of tracking an internal field, I would make it a method which is called after initializing the object. Also, I think both connect and auth can be combined into one. Nevertheless, I won't stand on it.
There was a problem hiding this comment.
_create_task may not be the best name.
It is not like "create this task", but "task named create".
Even if I add a method like:
async def ensure_connected(self):
await self._create_task
I would still have to call it with await:
connection = self._create_connection(translated)
await connection.ensure_connected()
So that doesn't buy us much.
| self, connection_manager, connection_id, address: Address, network_config, message_callback | ||
| ): | ||
| return await AsyncioConnection.create_and_connect( | ||
| return AsyncioConnection.create_and_connect( |
There was a problem hiding this comment.
The create_and_connect is not async anymore, it just returns the connection, without waiting for the connection to be established.
| async def _create_connection(self, config, address): | ||
| sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) | ||
| sock.setblocking(False) | ||
| sock.settimeout(0) |
There was a problem hiding this comment.
timeout 0? dont we have setting for it?
There was a problem hiding this comment.
No, the timeout is set to 0 to immediately return from socket creation.
It's the same with the asyncore client.
The connection_timeout is a separate setting.
|
@ihsandemir I'll merge this, but I'll address any concerns in a separate PR if necessary. |
yuce
changed the title
yuce
changed the title
5 participants
I've improved the stability of the asyncio client in this PR.
connection_manager_test.pyto asyncioAsyncioConnectionto be more reliable by porting more logic fromAsyncoreConnection.