Upstream support + example #11
Conversation
|
Hello @aranc! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-01-18 07:09:55 UTC |
aiofcm/connection.py
Outdated
| # type: (int, str, int, Optional[asyncio.AbstractEventLoop]) -> NoReturn | ||
| self.sender_id = sender_id | ||
| self.api_key = api_key | ||
| self.callback = callback | ||
| assert min_connections <= max_connections, "min_connections is greater than max_connections" |
There was a problem hiding this comment.
Let's change it from assert to ValueError exception because asserts could be disabled
aiofcm/connection.py
Outdated
| @@ -161,21 +196,27 @@ def is_busy(self): | |||
| class FCMConnectionPool: | |||
| MAX_ATTEMPTS = 10 | |||
|
|
|||
| def __init__(self, sender_id, api_key, max_connections=10, loop=None): | |||
| def __init__(self, sender_id, api_key, callback=None, min_connections=0, max_connections=10, loop=None): | |||
There was a problem hiding this comment.
Let's rename callback to more concrete upstream_callback here and in other places
aiofcm/connection.py
Outdated
| @@ -247,6 +288,19 @@ async def send_message(self, message: Message) -> MessageResponse: | |||
| logger.error('Could not send message %s: %s', | |||
| message.message_id, e) | |||
|
|
|||
| async def maintain_min_connections_open(self): | |||
| while True: | |||
There was a problem hiding this comment.
We need to stop maintaining connections if close() was called.
aiofcm/connection.py
Outdated
|
|
||
| await asyncio.sleep(1) | ||
|
|
||
| for connection in self.connections[-missing_connections:]: |
There was a problem hiding this comment.
If pool was closed it's better not to refresh connections here.
aiofcm/connection.py
Outdated
| self.max_connections = max_connections | ||
| self.loop = loop or asyncio.get_event_loop() | ||
| self.connections = [] | ||
| self._lock = asyncio.Lock(loop=self.loop) | ||
|
|
||
| self.loop.set_exception_handler(self.__exception_handler) | ||
|
|
||
| asyncio.ensure_future(self.maintain_min_connections_open()) |
There was a problem hiding this comment.
Let's save this task in the pool and cancel it in close method.
aiofcm/connection.py
Outdated
| ) | ||
| payload = FCMMessage() | ||
|
|
||
| payload_body = {"to":str(device_token), "message_id":str(message_id), "message_type":"ack"} |
There was a problem hiding this comment.
Let's make it a little bit more PEP8 friendly and add spaces between keys and values and split this dict to multiple lines because it's already has more than 80 characters.
aiofcm/connection.py
Outdated
| try: | ||
| await self.xmpp_client.stream.send(msg) | ||
| except Exception: | ||
| self.requests.pop(message.message_id) |
There was a problem hiding this comment.
NameError will be raised but anyway why do we need pop here? self.requests contains futures for downstream messages.
There was a problem hiding this comment.
Agree. The pop must be removed. (Therefore the try/except control-flow can also be removed)
Fixed.
aiofcm/connection.py
Outdated
| while True: | ||
| missing_connections = max(0, self.min_connections - len(self.connections)) | ||
| if missing_connections > 0: | ||
| logger.debug('Creating %d missing connections' % missing_connections) |
There was a problem hiding this comment.
Don't use string formatting in logger messages in this way because some things like Sentry would consider this as different events. So, replace percent with comma.
aiofcm/client.py
Outdated
| @@ -7,9 +7,12 @@ | |||
|
|
|||
|
|
|||
| class FCM: | |||
| def __init__(self, sender_id, api_key, max_connections=10, loop=None): | |||
| def __init__(self, sender_id, api_key, callback=None, min_connections=None, max_connections=10, loop=None): | |||
There was a problem hiding this comment.
Let's rename callback to upstream_callback.
|
No prob. Fixed style issues, will check other issues soon |
|
Done. Resolved all current issues in review |
|
Just to be cleaner, also adding a commit to "Don't start maintain_connections_task if min_connections==0" |
|
Hi, I added a commit to remove destroyed connections from the list of connections maintained by the connections pool. I'm not sure at all if this is the best way to approach this, but it fixes an issue I had with the upstream connection being destroyed without any new connection being opened to replace it. Some possible issues with this fix:
I'm opening this review for future discussion, probably not ready for merge yet |
Added upstream support: https://firebase.google.com/docs/cloud-messaging/receive-upstream