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

Fix deadlock where opening database fails #107

Merged
merged 10 commits into from
Jan 30, 2021
Merged

Conversation

padelt
Copy link

@padelt padelt commented Feb 21, 2020

...without setting self.exception; avoid deadlock after race condition where command is enqueued before thread can signal an exception #90

…ion; avoid deadlock after race condition where command is enqueued before thread can signal an exception piskvorky#90
@ronnymajani
Copy link

@mpenkov Any updates on getting this PR merged?

I've tested this PR in our own fork and it pretty much fixes the issue, and has been stable so far (been using this patch for a couple of months now).

Just though I'd ping you on this, as it would be useful patch for others to have
And thanks for this module, it's sweet, and easily extendable ❤️

sqlitedict.py Outdated
conn = sqlite3.connect(self.filename, check_same_thread=False)
except Exception as ex:
self.log.exception("Failed to initialize connection for filename: %s" % self.filename)
self.exception = (e_type, e_value, e_tb) = sys.exc_info()
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the point of declaring the new variables?

Suggested change
self.exception = (e_type, e_value, e_tb) = sys.exc_info()
self.exception = sys.exc_info()

sqlitedict.py Outdated
@@ -552,6 +573,28 @@ def close(self, force=False):
self.select_one('--close--')
self.join()

def wait_for_initialization(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's worth making this thing explicitly internal, so people don't touch it (or at least expect bad things to happen if they do).

Suggested change
def wait_for_initialization(self):
def _wait_for_initialization(self):

sqlitedict.py Outdated
@@ -390,19 +395,34 @@ def __init__(self, filename, autocommit, journal_mode):
self.reqs = Queue()
self.setDaemon(True) # python2.5-compatible
self.exception = None
self.initialized = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self.initialized = None
self._initialized = None

@mpenkov
Copy link
Collaborator

mpenkov commented Jan 22, 2021

Thank you for reminding me about this. Let's work on getting this merged. I left you some minor comments, please have a look.

@ronnymajani
Copy link

This isn't my PR, so I'll wait for the original author to jump in @padelt
But if they don't then I'll fork this and make a copy PR to get this merged in faster

@padelt
Copy link
Author

padelt commented Jan 24, 2021

Hey everyone - nearly forget about this PR.
@mpenkov Thanks for the comments. I just added two commits which should address those.
@ronnymajani Thank you for deliberately not stepping on toes. ;-) I don't mind whose PR gets merged.
Whatever is easier for you all!

@padelt
Copy link
Author

padelt commented Jan 24, 2021

Had to rename the variable again to avoid messing up threading.Thread's internal state.

@mpenkov Please have a look at the line # endclass SqliteMultithread - I inserted a space to please Flake8 - but I have no idea what that comment is supposed to do - so maybe it was important to have that exact syntax?

Tests now pass: https://github.com/padelt/sqlitedict/runs/1757442319

@mpenkov mpenkov merged commit 9cc029f into piskvorky:master Jan 30, 2021
@mpenkov
Copy link
Collaborator

mpenkov commented Jan 30, 2021

Merged. Thank you for your contribution and your patience @padelt !

@shawnboltz
Copy link

Hello, are there any plans to release this in a new version to PyPI? It appears to address an issue that I'm having with the latest official release. Thanks!

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.

4 participants