Skip to content

Conversation

@tkaemming
Copy link
Contributor

This isolates out the worker pool logic from the Riak client itself so
that its behavior is easier to understand.

The primary side effect of this change is that it causes the thread pool
to be set up when the first job is submitted, which should prevent a
deadlock that would manifest itself when the Riak client was
instantiated in the parent process of a multi-process environment.

This also makes some other minor changes for the sake of readability
(e.g. using partial rather than binding key and event to callback
function as default arguments, and unpacking the tuple argument provided
to SimpleThreadedWorkerPool.submit to ensure it is the correct length.)

This also fixes the same issue as GH-5411 (since otherwise it would
conflict.)

This isolates out the worker pool logic from the Riak client itself so
that its behavior is easier to understand.

The primary side effect of this change is that it causes the thread pool
to be set up when the first job is submitted, which should prevent a
deadlock that would manifest itself when the Riak client was
instantiated in the parent process of a multi-process environment.

This also makes some other minor changes for the sake of readability
(e.g. using `partial` rather than binding `key` and `event` to callback
function as default arguments, and unpacking the tuple argument provided
to `SimpleThreadedWorkerPool.submit` to ensure it is the correct length.)

This also fixes the same issue as GH-5411 (since otherwise it would
conflict.)
@tkaemming
Copy link
Contributor Author

tkaemming commented Jun 1, 2017

I'm not sure if there are any tests that specifically cover these paths (eek) but I did test this against a local Riak to ensure I didn't break anything in the move. I'm fairly confident this fixes the deadlock issue (ask me for more details about that if you want them.)

@getsentry-bot
Copy link
Contributor

1 Error
🚫 You need to update CHANGES due to the size of this PR

Generated by 🚫 danger

Copy link
Contributor

@mattrobenolt mattrobenolt left a comment

Choose a reason for hiding this comment

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

Yeah, I assume you actually ran this and it worked fine. Nothing seems wrong here.

@tkaemming tkaemming merged commit 29060e9 into master Jun 1, 2017
@tkaemming tkaemming deleted the lazy-riak-thread-pool branch June 1, 2017 23:50
tkaemming added a commit that referenced this pull request Jun 6, 2017
This should be unnecessary after GH-5499.

This is essentially a revert of ac5dfeb.
tkaemming added a commit that referenced this pull request Jun 6, 2017
This should be unnecessary after GH-5499.

This is essentially a revert of ac5dfeb.
@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants