Skip to content

Fix deadlock where opening database fails - #107

Merged
mpenkov merged 10 commits into
piskvorky:masterfrom
padelt:master
Jan 30, 2021
Merged

Fix deadlock where opening database fails #107
mpenkov merged 10 commits into
piskvorky:masterfrom
padelt:master

Conversation

@padelt

@padelt padelt commented Feb 21, 2020

Copy link
Copy Markdown

...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
Copy Markdown

@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 ❤️

Comment thread 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
Copy Markdown
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()
Comment thread sqlitedict.py Outdated
self.select_one('--close--')
self.join()

def wait_for_initialization(self):

Copy link
Copy Markdown
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):
Comment thread sqlitedict.py Outdated
self.reqs = Queue()
self.setDaemon(True) # python2.5-compatible
self.exception = None
self.initialized = None

Copy link
Copy Markdown
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

mpenkov commented Jan 22, 2021

Copy link
Copy Markdown
Collaborator

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
Copy Markdown

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

padelt commented Jan 24, 2021

Copy link
Copy Markdown
Author

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

padelt commented Jan 24, 2021

Copy link
Copy Markdown
Author

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

mpenkov commented Jan 30, 2021

Copy link
Copy Markdown
Collaborator

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

@shawnboltz

Copy link
Copy Markdown

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

5 participants