-
Notifications
You must be signed in to change notification settings - Fork 245
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
Use django.db close_old_connections in place of explicit connections.close #509
Conversation
This opens things up to connection reuse via `CONN_MAX_AGE` and otherwise gets the full django connection cleanup behavior instead of explicitly closing the connection for each event handled.
|
I don't sign legal documents as part of OSS contributions. CLA aren't necessary and are bad for contributors. Very few true OSS projects require them, I've only really seen them on company sponsored projects like this. They're bad practice, CLA vs DCO. Feel free to port/redo the trivial changes in this PR yourself as you see fit. |
Hi @ross, thanks a lot for taking the time to share this feedback and a suggestion to resolve the issue! I've checked
Regarding the CLA, we respect your opinion about it and thank you very much for saying this. We'll work on a new pull request based on your suggestion. If you are happy to use your time a little bit more, your reviews on our forthcoming pull request would be appreciated. |
Thanks. I was totally focused on the closing aspect and didn't even look at the start signal enough to see that it was calling the method as well. Definitely makes sense to do it similarly for slack_bolt even handlers.
Wfm. Happy to take a look at and test PRs. Thanks. |
Not directly related to this PR/issue, but after failing to find any information about using Django from |
@ross Thanks for sharing this! Your solution for this Django issue is very clean and looks great to me 👍 If your idea will be included out-of-the-box in the Django framework in the future (=encouraging a Django specific thread executor for threading), it should be so helpful not only for this Slack app use case but also for the entire community. |
Closed in favor of #512 |
I was looking to enable the django db
CONN_MAX_AGE
to avoid the overhead of reconnecting to the database (TLS encrypted) for every event being handled similar to how I have in the past with normal django web requests.I quickly realized this wasn't working as I continued to see the mysql connection counters on the server rising for each request. I dug into the log message about closing connections and ran across
release_thread_local_connections
bolt-python/slack_bolt/adapter/django/handler.py
Line 60 in 99ed6a6
That method appears to explicitly close all connections after each event is handled which makes
CONN_MAX_AGE
ineffective.This PR makes a relatively minor change to
release_thread_local_connections
to instead call the samerequest_finished
handler the signal mentioned in #280 (comment),django.db.close_old_connections
which appears to be easy/safe since it ignores all args with**kwargs
.This opens things up to connection reuse via
CONN_MAX_AGE
and otherwise gets the full django connection cleanup behavior on errors instead of explicitly closing the connection for each event handled./cc #280 #281 which introduced the explicit close
/cc @seratch who authored ^ and would likely know if this makes sense
Category (place an
x
in each of the[ ]
)slack_bolt.App
and/or its core componentsslack_bolt.async_app.AsyncApp
and/or its core componentsslack_bolt.adapter
/docs
Requirements (place an
x
in each[ ]
)Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.
./scripts/install_all_and_run_tests.sh
after making the changes.