fix: addresses event completion race condition in pubsub test utility #2332

jonathanj-square · 2024-08-12T21:21:08Z

The race conditions was caused by two issues.

The WaitGroup down tick was not synchronized with the completion of the subscriber verb execution. Failure to wait for completion may result in a down time coming before a new event is dispatched (which ticks up the waitgroup) - which may result in the WaitGroup reaching zero too soon.
Non-linear pubsub networks are not supported by simply counting up when an event is published and down after it is completed. The up count needs to match the number of live subscriptions (e.g. subscriptions with registered subscribers)

This synchronization scheme does not cover asynchronous event dispatch (e.g. events dispatched via go routines within the subscriber verb) such scenarios introduce a race condition that cannot be resolved by black box external synchronization.

… given the underlying issue is topology sensitive

…ce condition. issue #1 the waitgroup used to await event processing completion is not synchronized with event handling completion. The waitgroup has its count synchronously incremented before the event gets published but the down tick is not explicitly synchronized with handler completion. The down tick occurs in `handlePubSubEvent` in response to a `publishEvent` signal - this signals the intention to distribute an event on a topic not the completion. The `subscriptionDidConsumeEvent` is the right event to target. Given the event is dispatched via a goroutine the race condition may or may not trigger failure depending on competition execution timings. issue #2 event synchronization needs to consider the subscription topology in order to account for non-linear event propagation networks remaining issue synchronous event propagation from the subscriber verb is covered but not asynchronous propagation (e.g. a scenario where a verb spawns a goroutine that delays before writing an event to another topic might fire the event after the waitgroup reaches zero)

safeer

Looks great!

safeer · 2024-08-12T22:35:45Z

go-runtime/ftl/ftltest/testdata/go/pubsub/pubsub_test.go

matt2e

Great!

…ere are no live subscriptions.

jonathanj-square added 3 commits August 8, 2024 13:56

unit test repro of issue #2266

17048f4

revised unit test repro to capture multiple pubsub network topologies…

f7e4e99

… given the underlying issue is topology sensitive

jonathanj-square requested a review from alecthomas as a code owner August 12, 2024 21:21

jonathanj-square requested review from a team and safeer and removed request for a team August 12, 2024 21:21

ftl-robot mentioned this pull request Aug 12, 2024

Dashboard #728

Open

safeer approved these changes Aug 12, 2024

View reviewed changes

go-runtime/ftl/ftltest/testdata/go/pubsub/pubsub_test.go

Copy link

Contributor

safeer Aug 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

jonathanj-square added this pull request to the merge queue Aug 12, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 12, 2024

jonathanj-square added this pull request to the merge queue Aug 13, 2024