Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The listener can skip a message that commits after a message with a higher id, and the subscriber never receives it.
The race
Ids are assigned when an INSERT runs, but a row only becomes visible when its transaction commits, so two writers (two processes, or the writer thread of each) can commit out of id order:
id > last_id, reads 11, and moveslast_idto 11.id > 11, so row 10 is never read.Autocommit inserts keep the window short, but it is there on PostgreSQL and MySQL whenever there is more than one writer. SQLite serializes writers, so ids always commit in order there.
test/lib/action_cable/subscription_adapter/solid_cable_commit_order_test.rbholds writer A's transaction open to make the race deterministic. Onmainthe first two tests time out waiting for the late message on both PostgreSQL and MySQL.The fix
last_idnow only moves past rows that were read at leastlate_commit_windowago (default 1 second). Rows read more recently than that are left out of the next poll withid NOT IN (...)instead, so a row that commits late, with an id between them, is still returned.The per-channel value in
channelsused to be a cursor that advanced with every delivered message, which would also have dropped a late row whose id is below the channel's last delivered id. It is now only the subscribe-time watermark thatadd_channelalready sets: a channel receives rows with an id above the newest id when it subscribed, as before. Duplicate delivery is prevented by theNOT INlist, not by that cursor.Costs:
NOT INlist holds the ids read on subscribed channels during the lastlate_commit_window, so its size follows the traffic on those channels. Payloads are still read once.late_commit_windowafter a higher id was read is still skipped. The option is documented in the README.last_id; that is unchanged. When a channel subscribes again, the first poll still reads that channel's rows written since then and skips them by the watermark, as before. Their ids now also stay in theNOT INlist for onelate_commit_window, but their payloads are not read again.Overhead of the
NOT INlist on the poll query when there is nothing new to read. The table has 200k rows over 50 channels, 5 of them subscribed. "NOT INn" means n messages were read on subscribed channels during the last window. Local Docker, 2000 polls each:mainNOT IN10NOT IN100NOT IN1000With the default window, n is the number of messages per second on the channels one process subscribes to.
I also considered keeping
last_idmoving on every poll and remembering only the ids missing from the range read, re-reading those by primary key. That query stays cheap at any traffic, but it has to read the ids of every channel to tell a missing id from another channel's row, and it needs more code. I went with the simpler version, and can switch if you prefer the other trade-off.Testing
Tested locally with
bin/teston PostgreSQL 15.1, MySQL 8.0.31 and SQLite using the default Gemfile, and on PostgreSQL and MySQL usinggemfiles/rails_7_2.gemfile: all pass. The new tests are skipped on SQLite. The adapter tests passed five times in a row on PostgreSQL and MySQL.