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

Properly synchronize Mysql2Adapter#active? and TrilogyAdapter#active? #51012

Merged
merged 1 commit into from Feb 8, 2024

Conversation

casperisfine
Copy link
Contributor

As well as disconnect! and verify!.

This generally isn't a big problem as connections must not be shared between threads, but is required when running transactional tests or system tests and could lead to a SEGV.

Found while working on #50999

@casperisfine casperisfine force-pushed the fix-thread-safety-in-mysql-adapters branch from 3e50b8e to 6c4178d Compare February 8, 2024 13:58
…ve?`

As well as `disconnect!` and `verify!`.

This generally isn't a big problem as connections must not be shared between
threads, but is required when running transactional tests or system tests
and could lead to a SEGV.
@casperisfine casperisfine force-pushed the fix-thread-safety-in-mysql-adapters branch from 6c4178d to 6f3b46b Compare February 8, 2024 14:01
@byroot byroot merged commit 257c630 into rails:main Feb 8, 2024
3 of 4 checks passed
@casperisfine casperisfine deleted the fix-thread-safety-in-mysql-adapters branch February 8, 2024 14:13
@ngan
Copy link
Contributor

ngan commented Feb 9, 2024

Nice find, Jean. I think this fixes the problem we were seeing in our CI. Do you plan to backport this to 7.1 or 7.0? Going to apply a monkey patch in the meantime.

@casperisfine
Copy link
Contributor Author

Hum, I didn't backport because I didn't think it was possible to run into in outside of #50999.

But you guys set the lock_thread manually don't you? if so, yeah I can look at backporting it.

byroot added a commit that referenced this pull request Feb 9, 2024
…pters

Properly synchronize `Mysql2Adapter#active?` and `TrilogyAdapter#active?`
@byroot
Copy link
Member

byroot commented Feb 9, 2024

Backported to 7-1-stable as eb760da

7-0 would be much more painful though, so not super keen on it.

@ngan
Copy link
Contributor

ngan commented Feb 9, 2024

It’s ok. We’re going to 7.1 very soon.

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

Successfully merging this pull request may close these issues.

None yet

3 participants