Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Django connection issue with RTM v2 client #1238

Open
seratch opened this issue Jul 15, 2022 · 3 comments
Open

Django connection issue with RTM v2 client #1238

seratch opened this issue Jul 15, 2022 · 3 comments
Assignees
Labels
enhancement M-T: A feature request for new functionality rtm-client Version: 3x
Milestone

Comments

@seratch
Copy link
Member

seratch commented Jul 15, 2022

We are refitting to use the v2 RTM client and still observe some stale Django connection issues. Ie, new DB connections are made for each message that gets processed (thread), but they are apparently not being released. I have tried using the close_old_connections & also ensure_connection, but seems like it doesn't really help. I saw you did the changes for the slack-bolt thing here. Were there going to be similar changes to the v2 RTM client? Do you have any recommendations in the interim?

Originally posted by @pbrackin in slackapi/bolt-python#280 (comment)

@seratch seratch changed the title We are refitting to use the v2 RTM client and still observe some stale Django connection issues. Ie, new DB connections are made for each message that gets processed (thread), but they are apparently not being released. I have tried using the close_old_connections & also ensure_connection, but seems like it doesn't really help. I saw you did the changes for the slack-bolt thing here. Were there going to be similar changes to the v2 RTM client? Do you have any recommendations in the interim? Django connection issue with RTM v2 client Jul 15, 2022
@seratch seratch added Version: 2x enhancement M-T: A feature request for new functionality rtm-client labels Jul 15, 2022
@seratch seratch self-assigned this Jul 15, 2022
@seratch seratch added this to the 3.18.0 milestone Jul 15, 2022
@seratch
Copy link
Member Author

seratch commented Jul 15, 2022

Hi @pbrackin, thanks for sharing this. The connection releasing logic may not work in the case where your message listener receives a message from its bot user itself: https://github.com/slackapi/python-slack-sdk/blob/v3.17.2/slack_sdk/rtm_v2/__init__.py#L162-L164

We are happy to add some customization points for this use case but, can you verify whether the approach works for you?

    def process_message(self):
        release_thread_local_connections()  # ADDED
        try:
            raw_message = self.message_queue.get(timeout=1)
            if self.logger.level <= logging.DEBUG:
                self.logger.debug(f"A message dequeued (current queue size: {self.message_queue.qsize()})")

            if raw_message is not None:
                message: dict = {}
                if raw_message.startswith("{"):
                    message = json.loads(raw_message)

                def _run_message_listeners():
                    self.run_message_listeners(message)

                self.message_workers.submit(_run_message_listeners)
        except Empty:
            pass
        finally:  # ADDED
            release_thread_local_connections()  # ADDED

If this works well, we will add two callbacks like on_process_message_start and on_process_message_end to run release_thread_local_connections() without modifying the RTM client code.

@seratch seratch modified the milestones: 3.18.0, 3.x Jul 19, 2022
@pbrackin
Copy link

Thanks @seratch for following up on this. Sorry I didn't get to respond a little sooner. I will run this experiment when I get some time. But I wanted to also point out that I found a solution that seems to work ok. What I did was call connections.close_all() at the end of my message handler. Also I call close_old_connections() in my main loop which runs every 15 secs atm. We use MySQL for this project, and so I figured out that running sudo mysqladmin -i 1 processlist was a good way to see what is happening with the DB connections. Anyway, with those 2 Django calls in place, the connection list stopped growing with new messages coming in, and I no longer get the OperationalError(s) like "MySQL has gone away".
Before I figured out this solution, I was about to just move all the incoming messages to a Q and have the processing all done by a dedicated processing thread - which would probably be ideal from a DB connection standpoint since that would allow Django to recycle connections in the pool. The cost of that would be some additional delay in the processing for Q / d-Q stuff. But with my solution, everything seems ok now.
Anyway - let me know what you think of my approach. I will get back on adding in the code changes you suggested.

@seratch
Copy link
Member Author

seratch commented Aug 1, 2022

@pbrackin Thanks for your reply. Most of the things you mentioned should be good ways to go but

What I did was call connections.close_all() at the end of my message handler.

This should be close_old_connections for safety. Refer to slackapi/bolt-python#509 for more details.

Let me know if my above suggestions work for you. Once it is effective, we are happy to enhance the RTM v2 client to enable developers to pass callback functions for this use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement M-T: A feature request for new functionality rtm-client Version: 3x
Projects
None yet
Development

No branches or pull requests

2 participants