pyslackers / slack-sansio

Python (a)sync Slack API library
MIT License
37 stars 8 forks source link

Needs pinging to keep bot(s) alive #50

Open Battleroid opened 5 years ago

Battleroid commented 5 years ago

This may need some sort of periodic task that sends a ping over the websocket to prevent bots from disconnecting. See "Ping and Pong".

I've seen it done like the following:

async def keepalive(self):
    while True:
        log.info(f'sending ping {self._ping_count}')
        await self.ping_ws()

async def ping_ws(self):
    await asyncio.sleep(60)
    self._ping_count += 1

    await self.ws.send(
        json.dumps({
            'id': self._ping_count,
            'type': 'ping'
        })
    )

with the keepalive task created shortly after startup like so:

self.loop.create_task(self.keepalive())
Battleroid commented 5 years ago

~I ended up hackily putting something together that did the above in my bot that uses slack-sansio and it seems to have to kept it from disconnecting.~ I spoke too soon. Should probably expose the websocket connection on the slack object somewhere.

ovv commented 5 years ago

Thanks for the link, I don't think that existed at the time. Do you know if this is required or optional ? I will have to thing of a way to include that for all IO implementation.

In the meantime I suggest you subclass the aiohttp implementation and change the _rtm function to expose the websocket object. Something like:

from slack.io.aiohttp import SlackAPI

class ClientWithWS:
    async def _rtm:
        async with self._session.ws_connect(url) as self.ws:
            async for data in self.ws:
                if data.type == aiohttp.WSMsgType.TEXT:
                    yield data.data
                elif data.type == aiohttp.WSMsgType.CLOSED:
                    break
                elif data.type == aiohttp.WSMsgType.ERROR:
                    break
Battleroid commented 5 years ago

I'm not sure honestly. I would assume it's required as my bots generally stop responding after a time.

On December 6, 2018 1:15:36 PM EST, Quentin Dawans notifications@github.com wrote:

Thanks for the link, I don't think that existed at the time. Do you know if this is required or optional ? I will have to thing of a way to include that for all IO implementation.

In the meantime I suggest you subclass the aiohttp implementation and change the _rtm function to expose the websocket object. Something like:

from slack.io.aiohttp import SlackAPI

class ClientWithWS:
   async def _rtm:
       async with self._session.ws_connect(url) as self.ws:
           async for data in self.ws:
               if data.type == aiohttp.WSMsgType.TEXT:
                   yield data.data
               elif data.type == aiohttp.WSMsgType.CLOSED:
                   break
               elif data.type == aiohttp.WSMsgType.ERROR:
                   break

Casey Weed

ovv commented 5 years ago

Ok thanks, I'll have to test it out. I have stopped using the websocket connection a long time ago as the HTTP events API is much more reliable (on a side note there is a linked project helping with that: https://github.com/pyslackers/sir-bot-a-lot-2/)

Battleroid commented 5 years ago

Cool, thank you.

On December 6, 2018 1:18:57 PM EST, Quentin Dawans notifications@github.com wrote:

Ok thank, I'll have to test it out. I have stopped using the websocket connection a long time ago as the HTTP events API is much more reliable (on a side note there is a linked project helping with that: https://github.com/pyslackers/sir-bot-a-lot-2/)

Casey Weed

iMerica commented 5 years ago

Most mature websocket clients & servers do ping/pong health checks automatically.

Battleroid commented 5 years ago

@iMerica in that case it might just be something broke for my bot instead, in which case this issue can probably be ignored

ovv commented 5 years ago

@iMerica it's a custom slack JSON ping. From the documentation it looks like it's optional. I think the issue is slack closing the connection after a while, a goodbye event should be received before closing.

Not sure if adding the ping would tell slack to not close it :thinking: