-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add PostgreSQL cross-process wake-up #14
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
Changes from all commits
33694bc
9a6ca78
f61792e
e93724c
de042d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| # rbs_inline: enabled | ||
|
|
||
| module SolidObjects | ||
| module WakeUpAdapters | ||
| module_function | ||
|
|
||
| # Returns the best wake-up strategy for a connection: cross-process | ||
| # notifications where the database provides them, and the in-process | ||
| # default everywhere else. | ||
| # | ||
| # This is deliberately not the default. A notification adapter opens a | ||
| # connection per waiting thread outside the pool, and `LISTEN` does not | ||
| # survive a transaction-pooling proxy such as PgBouncer, so adopting it is | ||
| # a deployment decision rather than an upgrade side effect. | ||
| # | ||
| # @rbs (?untyped) -> untyped | ||
| def for(connection = Record.connection) | ||
| return Postgresql.new if connection.adapter_name.match?(/postgres/i) | ||
|
|
||
| WakeUp.new | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| # rbs_inline: enabled | ||
|
|
||
| module SolidObjects | ||
| module WakeUpAdapters | ||
| # Wakes runtime roles across processes using PostgreSQL notifications. | ||
| # | ||
| # The in-process wake-up cannot reach another process, so a commit in a web | ||
| # process leaves a worker waiting out its polling interval. This adapter | ||
| # keeps that polling interval as the upper bound and delivers a notification | ||
| # when one is available, so a missed or failed notification costs latency | ||
| # rather than correctness. | ||
| class Postgresql | ||
| CHANNEL = "solid_objects_wake_up" | ||
| FAILED_WAIT_INTERVAL = 0.05 | ||
|
|
||
| # @rbs @channel: String | ||
| # @rbs @mutex: Thread::Mutex | ||
| # @rbs @connections: Array[untyped] | ||
|
|
||
| attr_reader :channel | ||
|
|
||
| # @rbs (?channel: String) -> void | ||
| def initialize(channel: CHANNEL) | ||
| @channel = channel | ||
| @mutex = Thread::Mutex.new | ||
| @connections = [] | ||
| end | ||
|
|
||
| # @rbs () -> bool | ||
| def signal | ||
| notify_channel | ||
| true | ||
| rescue => error | ||
| instrument_failure(:signal, error) | ||
| false | ||
| end | ||
|
|
||
| # @rbs (timeout: Numeric) -> bool | ||
| def wait(timeout:) | ||
| connection = listening_connection | ||
| !connection.raw_connection.wait_for_notify(timeout.to_f).nil? | ||
| rescue => error | ||
| instrument_failure(:wait, error) | ||
| pace_after_failure(timeout) | ||
| false | ||
| end | ||
|
|
||
| # Starts listening before a caller blocks, so a notification sent between | ||
| # startup and the first wait is not missed. | ||
| # @rbs () -> bool | ||
| def listen | ||
| listening_connection | ||
| true | ||
| rescue => error | ||
| instrument_failure(:listen, error) | ||
| false | ||
| end | ||
|
|
||
| # @rbs () -> bool | ||
| def stop | ||
| open = mutex.synchronize do | ||
| listening = connections.dup | ||
| connections.clear | ||
| listening | ||
| end | ||
| Thread.current[thread_key] = nil | ||
| open.each { |connection| disconnect(connection) } | ||
| open.any? | ||
| end | ||
|
|
||
| private | ||
|
|
||
| attr_reader :mutex, :connections | ||
|
|
||
| # @rbs () -> void | ||
| def notify_channel | ||
| Record.connection_pool.with_connection do |connection| | ||
| connection.execute("NOTIFY #{connection.quote_table_name(channel)}") | ||
| end | ||
| end | ||
|
|
||
| # A listening connection is dedicated and per thread. `LISTEN` is per | ||
| # connection, a blocking wait must not hold a connection the rest of the | ||
| # runtime needs, and one connection cannot serve concurrent waiters: the | ||
| # supervisor shares one adapter across roles, and a notification consumed | ||
| # by one waiter would leave the others asleep until their poll expired. | ||
| # @rbs () -> untyped | ||
| def listening_connection | ||
| connection = Thread.current[thread_key] | ||
| return connection if connection&.active? | ||
|
|
||
| open_listening_connection | ||
| end | ||
|
|
||
| # @rbs () -> untyped | ||
| def open_listening_connection | ||
| connection = Record.connection_pool.send(:new_connection) | ||
| connection.execute("LISTEN #{connection.quote_table_name(channel)}") | ||
| Thread.current[thread_key] = connection | ||
| mutex.synchronize { connections << connection } | ||
|
Comment on lines
+97
to
+100
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a configured PostgreSQL adapter has opened per-thread listeners and the supervisor is restarted in the same process, Prompt To Fix With AIThis is a comment left during a code review.
Path: lib/solid_objects/wake_up_adapters/postgresql.rb
Line: 97-100
Comment:
**Listener connections survive shutdown**
When a configured PostgreSQL adapter has opened per-thread listeners and the supervisor is restarted in the same process, `Supervisor#stop` never invokes the adapter's `stop`, so those dedicated connections remain open and accumulate across restarts, eventually consuming PostgreSQL connection capacity.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid. The adapter holds connections outside the pool and Fixed in f61792e: shutdown stops the wake-up adapter when it supports Regression test: configure the adapter, open a listener, start and stop a supervisor, and assert no connections remain tracked. |
||
| connection | ||
| end | ||
|
|
||
| # @rbs () -> Symbol | ||
| def thread_key | ||
| :"solid_objects_wake_up_#{object_id}" | ||
| end | ||
|
|
||
| # @rbs (untyped) -> void | ||
| def disconnect(connection) | ||
| connection.disconnect! | ||
| rescue | ||
| nil | ||
| end | ||
|
|
||
| # @rbs (Numeric) -> void | ||
| def pace_after_failure(timeout) | ||
| interval = [ timeout.to_f, FAILED_WAIT_INTERVAL ].min | ||
| return unless interval.positive? | ||
|
|
||
| sleep interval | ||
| end | ||
|
|
||
| # @rbs (Symbol, Exception) -> void | ||
| def instrument_failure(operation, error) | ||
| SolidObjects.instrument( | ||
| :"wake_up.failed", | ||
| adapter: "postgresql", | ||
| operation: operation.to_s, | ||
| error_class: error.class.name, | ||
| error_message: error.message | ||
| ) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| # Generated from lib/solid_objects/wake_up_adapters.rb with RBS::Inline | ||
|
|
||
| module SolidObjects | ||
| module WakeUpAdapters | ||
| # Returns the best wake-up strategy for a connection: cross-process | ||
| # notifications where the database provides them, and the in-process | ||
| # default everywhere else. | ||
| # | ||
| # This is deliberately not the default. A notification adapter opens a | ||
| # connection per waiting thread outside the pool, and `LISTEN` does not | ||
| # survive a transaction-pooling proxy such as PgBouncer, so adopting it is | ||
| # a deployment decision rather than an upgrade side effect. | ||
| # | ||
| # @rbs (?untyped) -> untyped | ||
| def self?.for: (?untyped) -> untyped | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the supervisor runs multiple runtime roles, the process-wide adapter returns the same cached PostgreSQL connection to every waiter and calls
wait_for_notifyoutside the mutex. A notification is consumed through that one connection instead of independently waking every role, while concurrent connection use can also fail and leave roles asleep until their polling timeout.Prompt To Fix With AI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct on both halves, and the test half is the part I got wrong.
The adapter is memoized process-wide and shared across runtime roles, so every waiter used one cached connection and called
wait_for_notifyon it concurrently. Whichever waiter won consumed the notification and the other roles slept until their polling timeout, which defeats the purpose of the adapter, and concurrent use of a single connection is unsafe regardless.Fixed in 9a6ca78: a listening connection is opened and tracked per thread, and
stopdisconnects all of them.On the test: you were right that it only demonstrated multiple waiters with separate adapters. I rewrote it to share one adapter, and it still passed against the broken code, because it asserted only that each waiter woke, not when. Two of the three were waking at the 5 second timeout, which the assertion happily accepted. It now asserts every waiter wakes in under a second against a 5 second timeout. Against the shared connection that test hangs; with the per-thread connection all seven pass.
Added a second test asserting two threads hold distinct listening connections.