Forward sql.active_record notifications back into the calling thread#41590
Forward sql.active_record notifications back into the calling thread#41590eileencodes merged 1 commit intorails:mainfrom
Conversation
aab3870 to
33d1a24
Compare
There was a problem hiding this comment.
Since the notification is no longer triggered in the background thread, we can no longer use a monitor to do synchronization. The best I could think of is a sleep loop that check the FutureResult state.
33d1a24 to
619d8a6
Compare
There was a problem hiding this comment.
This might be a bit excessive, but I'm worried about a killed thread leaving a connection in a broken state.
3173ea1 to
ecccdca
Compare
jhawthorn
left a comment
There was a problem hiding this comment.
Small suggestions. I really like the approach 👍
activerecord/lib/active_record/connection_adapters/mysql/database_statements.rb
Outdated
Show resolved
Hide resolved
93505ee to
fbe5c56
Compare
It is not uncommon for `sql.active_record` subscribers to rely on thread local or fiber local state. For instance the `buffered-logger` gem buffer the logs in a thread variable. With the introduction of async queries, the `sql.active_record` events can now be produced from a background thread and that break some expectations. This makes it hard for subscriber to map the event to the request or job that scheduled it. That is why I believe we should instead store the event and publish it back on the calling thread when the results are accessed.
fbe5c56 to
091dc78
Compare
| elsif canceled? | ||
| @event_buffer&.flush | ||
|
|
||
| if canceled? |
There was a problem hiding this comment.
I changed the order here to avoid a flaky test. The cancelation test would sometimes fail because the query would be executed before we access the result.
But either way I think it makes sense to disallow accessing a canceled result, even if we have it.
|
CI is failing for an unrelated reason. |
| begin | ||
| if pending? | ||
| execute_query(connection, async: true) | ||
| @event_buffer = @instrumenter.buffer |
It is not uncommon for
sql.active_recordsubscribers to rely onthread local or fiber local state. For instance the
buffered-loggergem buffer the logs in a thread variable.
With the introduction of async queries, the
sql.active_recordevents can now be produced from a background thread and that break
some expectations.
This makes it hard for subscriber to map the event to the request
or job that scheduled it.
That is why I believe we should instead store the event and
publish it back on the calling thread when the results are
accessed.
cc @rafaelfranca @etiennebarrie @eileencodes @jhawthorn @tenderlove
NB: this will conflict a bit with #41586, once one is merged the other will have to rebase.