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

Monitor main_thread in Database/AsyncDatabase to determine whether we need to automatically call close() #214

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

blast-hardcheese
Copy link
Collaborator

@blast-hardcheese blast-hardcheese commented Apr 3, 2024

Why

Especially with default_db, it's not clear that the user still needs to call close(). Monitor the main thread for termination to do this automatically.

What changed

The presumption is that users won't be relying on the database to hold the python runtime open, and that any instance of that is in fact some form of error.

Test plan

>>> from replit.database import db
>>> ^D

confirm that we exit cleanly and quickly.

Rollout

No special instructions.

  • This is fully backward and forward compatible

@blast-hardcheese blast-hardcheese requested a review from a team as a code owner April 3, 2024 20:29
@blast-hardcheese blast-hardcheese requested review from cdmistman and removed request for a team April 3, 2024 20:29
@blast-hardcheese
Copy link
Collaborator Author

Further testing revealed that the way we do threading doesn't work with atexit. Trying to find a workaround now.

@blast-hardcheese blast-hardcheese force-pushed the dstewart/bug/atexit-db.close-hooks branch from 32ff509 to 3b374ab Compare April 3, 2024 21:36
@blast-hardcheese
Copy link
Collaborator Author

Ok. New approach.

Spawn a watchdog that monitors main_thread() so we know when to automatically call self.close().

@blast-hardcheese blast-hardcheese force-pushed the dstewart/bug/atexit-db.close-hooks branch from 3b374ab to 18261c5 Compare April 3, 2024 21:43
@blast-hardcheese blast-hardcheese changed the title Hook atexit.register/unregister in Async/Database Monitor main_thread in Database/AsyncDatabase to determine whether we need to automatically call close() Apr 3, 2024
@blast-hardcheese blast-hardcheese force-pushed the dstewart/bug/atexit-db.close-hooks branch from 18261c5 to 1a91db1 Compare April 3, 2024 22:55
Copy link

@lincoln-replit lincoln-replit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some kind of test for this would be awesome but otherwise LGTM!

@blast-hardcheese blast-hardcheese merged commit bbbaf0f into master Apr 4, 2024
6 checks passed
@blast-hardcheese blast-hardcheese deleted the dstewart/bug/atexit-db.close-hooks branch April 4, 2024 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants