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

Distribute connections to previously blocked threads when we're done #27108

Merged
merged 1 commit into from Nov 24, 2016

Conversation

Projects
None yet
2 participants
@matthewd
Member

matthewd commented Nov 19, 2016

Two methods block new connections; we were already doing the right thing for clear_reloadable_connections, but it's better placed in with_new_connections_blocked, where it can work for disconnect too.

cc @thedarkone

Follow-up to #27057, to address a nearby-but-separate issue we spotted while looking at that.

@matthewd matthewd self-assigned this Nov 19, 2016

# it to timeout with ConnectionTimeoutError
if (group_action_method == :disconnect || group_action_method == :disconnect!) && pool.num_waiting_in_queue > 0
pool.with_connection {} # create a new connection in case there are threads still stuck in a queue
end

This comment has been minimized.

@matthewd

matthewd Nov 19, 2016

Member

This change turned out to be very easy to test 😊

activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb Outdated
@@ -699,6 +680,8 @@ def checkout_for_exclusive_access(checkout_timeout)
end
def with_new_connections_blocked
num_new_conns_required = 0

This comment has been minimized.

@kamipo

kamipo Nov 19, 2016

Member

num_new_conns_required is only used in ensure block.

Distribute connections to previously blocked threads when we're done
Two methods block new connections; we were already doing the right thing
for clear_reloadable_connections, but it's better placed in
with_new_connections_blocked, where it can work for disconnect too.

@matthewd matthewd force-pushed the matthewd:allocate-connections-after-blocking branch to d314646 Nov 24, 2016

@matthewd matthewd merged commit 3b13761 into rails:master Nov 24, 2016

1 of 2 checks passed

codeclimate Code Climate is analyzing this code.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

matthewd added a commit that referenced this pull request Nov 24, 2016

Merge pull request #27108 from matthewd/allocate-connections-after-bl…
…ocking

Distribute connections to previously blocked threads when we're done
@matthewd

This comment has been minimized.

Member

matthewd commented Nov 24, 2016

Backported as b79c7fe

@matthewd matthewd deleted the matthewd:allocate-connections-after-blocking branch Jun 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment