Skip to content

Deliver messages that commit after a higher id - #91

Open
ya-luotao wants to merge 1 commit into
rails:mainfrom
ya-luotao:deliver-late-committed-messages
Open

ya-luotao wants to merge 1 commit into
rails:mainfrom
ya-luotao:deliver-late-committed-messages

Conversation

@ya-luotao

Copy link
Copy Markdown

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:

  1. Writer A inserts and gets id 10, not committed yet.
  2. Writer B inserts id 11 and commits.
  3. The listener polls id > last_id, reads 11, and moves last_id to 11.
  4. Writer A commits id 10. Every later poll reads 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.rb holds writer A's transaction open to make the race deterministic. On main the first two tests time out waiting for the late message on both PostgreSQL and MySQL.

The fix

last_id now only moves past rows that were read at least late_commit_window ago (default 1 second). Rows read more recently than that are left out of the next poll with id NOT IN (...) instead, so a row that commits late, with an id between them, is still returned.

The per-channel value in channels used 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 that add_channel already sets: a channel receives rows with an id above the newest id when it subscribed, as before. Duplicate delivery is prevented by the NOT IN list, not by that cursor.

Costs:

  • The NOT IN list holds the ids read on subscribed channels during the last late_commit_window, so its size follows the traffic on those channels. Payloads are still read once.
  • A late message arrives after higher ids on its channel. ActionCable's worker pool can already reorder callbacks, so subscribers do not rely on id order.
  • A message that commits more than late_commit_window after a higher id was read is still skipped. The option is documented in the README.
  • A listener with no subscribed channels does not move 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 the NOT IN list for one late_commit_window, but their payloads are not read again.

Overhead of the NOT IN list on the poll query when there is nothing new to read. The table has 200k rows over 50 channels, 5 of them subscribed. "NOT IN n" means n messages were read on subscribed channels during the last window. Local Docker, 2000 polls each:

PostgreSQL 15.1 MySQL 8.0.31
main 0.09 ms 0.12 ms
NOT IN 10 0.12 ms 0.27 ms
NOT IN 100 0.36 ms 0.18 ms
NOT IN 1000 2.70 ms 0.75 ms

With the default window, n is the number of messages per second on the channels one process subscribes to.

I also considered keeping last_id moving 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/test on PostgreSQL 15.1, MySQL 8.0.31 and SQLite using the default Gemfile, and on PostgreSQL and MySQL using gemfiles/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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant