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

ConnectionPool.connections provides public unsynchronized access to @connections #36465

Closed
jeffdoering opened this issue Jun 11, 2019 · 1 comment · Fixed by #36473 or hanneskaeufler/blog#174

Comments

@jeffdoering
Copy link
Contributor

ConnectionPool documentation is very crisp on the need to synchronize access to @connections but also states that public methods do not require synchronization:

But this attribute reader appears to blur that definition by creating a public method that directly exposes @connections

attr_reader :spec, :connections, :size, :reaper

This method shows what looks like the correct access pattern.

# Returns true if a connection has already been opened.

synchronize { @connections.any? }

The documentation should either clarify the requirement to synchronize around access to connections or the public connections reader should be replaced with a method that copies the array so that concurrent enumeration is safe (albeit holding connections in arbitrary states).

We noticed this because DataDog is searching for connections by object_id on Rails < 6 when this PR gave direct access: Pass the connection to the @instrumenter.instrument method call #34602

The original need was described here: Pass the connection to the @instrumenter.instrument method call #34568

But with that method added the fallback code is doing:

::ActiveRecord::Base.connection_handler.connection_pool_list .flat_map(&:connections).find { |c| c.object_id == connection_id }

https://github.com/DataDog/dd-trace-rb/blob/2e0a3b2d6b8dbf4aa609c1e47ddd4ec2a45a018f/lib/ddtrace/contrib/active_record/utils.rb#L46

Old (ancient) notes even suggested this kind of pattern:

Include the database configuration in activerecord notifications. #11284

The reference to DataDog here is just a concrete example. The request here is to either clarify the documentation on safe public methods or change the way connections is implemented so it meets the current documentation.

@rafaelfranca
Copy link
Member

I believe we should implement the method to copy the original array. Can you open a PR?

jeffdoering added a commit to instacart/rails that referenced this issue Jun 13, 2019
ConnectionPool documentation is clear on the need to synchronize
access to @connections but also states that public methods do not
require synchronization. Existing code exposed @connections
directly via attr_reader. The fix uses synchronize() to lock
@connections then returns a copy to the caller using Array.dup().

Includes comments on the connections method that thread-safe access
to the connections array does not imply thread-safety of accessing
methods on the actual connections.

Adds a test-case that modifies the pool using a supported method
in one thread  while a second thread accesses pool.connections.
The test fails without this patch.

Fixes rails#36465.
rafaelfranca pushed a commit that referenced this issue Jun 13, 2019
* Make ActiveRecord `ConnectionPool.connections` thread-safe.

ConnectionPool documentation is clear on the need to synchronize
access to @connections but also states that public methods do not
require synchronization. Existing code exposed @connections
directly via attr_reader. The fix uses synchronize() to lock
@connections then returns a copy to the caller using Array.dup().

Includes comments on the connections method that thread-safe access
to the connections array does not imply thread-safety of accessing
methods on the actual connections.

Adds a test-case that modifies the pool using a supported method
in one thread  while a second thread accesses pool.connections.
The test fails without this patch.

Fixes #36465.

* Update activerecord/test/cases/connection_pool_test.rb

[jeffdoering + Rafael Mendonça França]
rafaelfranca pushed a commit that referenced this issue Jun 13, 2019
* Make ActiveRecord `ConnectionPool.connections` thread-safe.

ConnectionPool documentation is clear on the need to synchronize
access to @connections but also states that public methods do not
require synchronization. Existing code exposed @connections
directly via attr_reader. The fix uses synchronize() to lock
@connections then returns a copy to the caller using Array.dup().

Includes comments on the connections method that thread-safe access
to the connections array does not imply thread-safety of accessing
methods on the actual connections.

Adds a test-case that modifies the pool using a supported method
in one thread  while a second thread accesses pool.connections.
The test fails without this patch.

Fixes #36465.

* Update activerecord/test/cases/connection_pool_test.rb

[jeffdoering + Rafael Mendonça França]
rafaelfranca pushed a commit that referenced this issue Jun 13, 2019
* Make ActiveRecord `ConnectionPool.connections` thread-safe.

ConnectionPool documentation is clear on the need to synchronize
access to @connections but also states that public methods do not
require synchronization. Existing code exposed @connections
directly via attr_reader. The fix uses synchronize() to lock
@connections then returns a copy to the caller using Array.dup().

Includes comments on the connections method that thread-safe access
to the connections array does not imply thread-safety of accessing
methods on the actual connections.

Adds a test-case that modifies the pool using a supported method
in one thread  while a second thread accesses pool.connections.
The test fails without this patch.

Fixes #36465.

* Update activerecord/test/cases/connection_pool_test.rb

[jeffdoering + Rafael Mendonça França]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants