fix (LiveViewTest): Support configurable timeouts on :async_pids GenServer calls to reduce test flakiness - #4425
Open
s3cur3 wants to merge 1 commit into
Open
fix (LiveViewTest): Support configurable timeouts on :async_pids GenServer calls to reduce test flakiness#4425s3cur3 wants to merge 1 commit into
:async_pids GenServer calls to reduce test flakiness#4425s3cur3 wants to merge 1 commit into
Conversation
…nServer calls to reduce test flakiness
A common source of test flakiness for our LiveView app at work (which heavily uses async assigns) looks like this:
```
1) test the page loads (MyAppWeb.AccountSettingsLiveTest)
lib/my_app_web/live/account_settings_live_test.exs:123
** (EXIT from #PID<0.12267.0>) exited in: GenServer.call(#PID<0.24640.0>, {:phoenix, :async_pids}, 5000)
** (EXIT) time out
```
That error is the result of a `render_async/1` call that timed out (though since it doesn't include a stacktrace, it takes a little effort to figure that out). Given that CI machines tend to be under-powered compared to developer machines, my understanding is that this occurs when the CPU is overloaded and takes longer than 5 seconds to fetch all the async processes attached to the LiveView.
The changes here are designed to both
a) allow clients to increase that timeout, and
b) present a clear error when a timeout does occur here.
The biggest part I'm not sure of: should the timeout provided to `render_async/2` be _split_ between the `:async_pids` lookup and the actual message receive (as written), or should it only apply to the message receive portion? Prior to this change, it was the latter, which means the whole function call could have taken up to 5 seconds + the indicated timeout. If the timeout is _not_ shared between the two steps, though, it feels quite awkward; either there's no way to configure the `:async_pids` timeout on a per-call basis (you would only be able to do so using the application config variable), or the function would need to accept *two* timeouts.
s3cur3
commented
Sep 4, 2026
Comment on lines
+1068
to
+1069
| timeout_ref = make_ref() | ||
| Process.send_after(self(), {timeout_ref, :timeout}, timeout) |
Contributor
Author
There was a problem hiding this comment.
Moved to starting the timeout before the GenServer call, so that the single timeout is shared between both the process lookup and the actual async work. (See PR description for a question on the tradeoffs here.)
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.
A common source of CI test flakiness for our LiveView app at work (which heavily uses async assigns) looks like this:
That error is the result of a
render_async/1call that timed out (though since it doesn't include a stacktrace, it takes a little effort to figure that out). Given that CI machines tend to be under-powered compared to developer machines, my understanding is this occurs when the CPU is overloaded and takes longer than 5 seconds to fetch all the async processes attached to the LiveView.The changes here are designed to both
a) allow clients to increase that timeout, and
b) present a clear error when a timeout does occur here.
The biggest part I'm not sure of: should the timeout provided to
render_async/2be split between the:async_pidslookup and the actual message receive (as written), or should it only apply to the message receive portion? Prior to this change, it was the latter, which means the whole function call could have taken up to 5 seconds + the indicated timeout. If the timeout is not shared between the two steps, though, it feels quite awkward; either there's no way to configure the:async_pidstimeout on a per-call basis (you would only be able to do so using the application config variable), or the function would need to accept two timeouts.