Replace hand-rolled retry/timeout plumbing with RxSharp - #3520
Merged
Conversation
…orkIdle with RxSharp Upstream Puppeteer builds Locator actions and waitForNetworkIdle on rxjs; we've been reimplementing the same retry/timeout/cancellation and debounce logic by hand in C#. Locator.RunWithRetryAsync was a manual retry loop with a linked CancellationTokenSource and several catch clauses just to tell timeout apart from cancellation apart from "retry again." WaitForNetworkIdleAsync hand-rolled the same debounce-on-events pattern with a System.Timers.Timer. Both now go through RxSharp (github.com/hardkoded/ReactiveExtensions-Sharp), a faithful RxJS port built for exactly this kind of swap. Locator uses its RetryAndRaceWithSignalAndTimer combinator; WaitForNetworkIdleAsync uses a BehaviorSubject driving DistinctUntilChanged + SwitchMap to express the same "wait for idleTime after the last change" debounce declaratively. Same external behavior and exception messages/types as before - all 39 Locator tests and 8 WaitForNetworkIdle tests pass unchanged, plus the full suite. RxSharp needed a strong-name-signed release (0.1.1) since PuppeteerSharp signs its own assembly and referencing an unsigned dependency fails the build under TreatWarningsAsErrors. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K92eapPm8e7mX7puT4cF4s
…/Frame/Target with RxSharp Same motivation as the Locator/WaitForNetworkIdle swap: upstream Puppeteer solves this exact "wait for an event, race it against a timeout and session-close" problem with firstValueFrom + raceWith on fromEmitterEvent streams, while we were hand-rolling it per method with TaskCompletionSource + manual event handler removal + WithTimeout. CdpPage.WaitForRequestAsync/WaitForResponseAsync/WaitForFrameAsync and Browser.WaitForTargetAsync now go through the same RxSharp combinators. Extracted two small shared helpers (TimeoutSignal, SessionClosedSignal) in CdpPage since three of the four methods needed the identical timeout/session-closed race branches - WaitForNetworkIdleAsync now reuses them too instead of duplicating the same construction. Found and fixed a real bug along the way: WaitForTargetAsync and WaitForFrameAsync originally used a plain Subject to bridge the raw event handlers into the Rx pipeline. A plain Subject has no buffer, so if the matching event fired between attaching the handler and this method actually subscribing via FirstValueFrom() (a real window, since CDP events arrive on their own thread), the emission was silently dropped and the call would hang until timeout - reproduced this against every page creation (Browser.WaitForTargetAsync is used internally by CreatePageInContextAsync). Fixed by using a 1-buffered ReplaySubject instead, which replays the value to the late subscriber. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K92eapPm8e7mX7puT4cF4s
kblok
changed the title
Bumps to RxSharp 0.1.2 (pending publish - see hardkoded/ReactiveExtensions-Sharp#1), which adds three primitives that let these two methods read much closer to upstream's merge(...).pipe(filterAsync(predicate), raceWith(...)) shape instead of the ReplaySubject + manual predicate-in-the-handler version from the previous commit: - FromEventBuffered eagerly attaches the raw event handler (instead of a plain Subject fed by a raw handler) and exposes it as a proper Observable, so predicate filtering can happen declaratively downstream via .Filter(predicate) instead of inside the handler. - RaceWithSignalAndTimer replaces the hand-built cancellation/timeout race branches in WaitForTargetAsync. - The private NeverReached helper duplicated in both Browser.cs and CdpPage.cs is gone, replaced by RxSharp's own AssumeNeverEmits. Same external behavior as before - same exception types/messages. Verified with 8 repeated reruns of the two tests that first exposed the ReplaySubject race, confirming FromEventBuffered doesn't reintroduce it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K92eapPm8e7mX7puT4cF4s
RxSharp's FromEventBufferedExtras/TimeoutExtras/CancellationExtras/etc are now one public static partial class PuppeteerExtras - see hardkoded/ReactiveExtensions-Sharp#1 for why. Mechanical follow-up: this repo's two call sites. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K92eapPm8e7mX7puT4cF4s
PuppeteerExtras -> Extensions, see hardkoded/ReactiveExtensions-Sharp#1. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K92eapPm8e7mX7puT4cF4s
Code review caught it: FromEventBuffered's default bufferSize of 1 means that if a matching event and a later non-matching event both land in the narrow gap between attaching the handler and the Rx pipeline actually subscribing, the size-1 buffer keeps only the non-matching one - silently dropping the match. The old TrySetResult-based implementation didn't have this risk (idempotent, first match always wins regardless of how many events fire before anyone awaits it). Fixed by passing an explicit EventBufferSize (16) instead of relying on the default - enough headroom to safely absorb a realistic burst in that gap without buffering unboundedly for the whole wait (once subscribed, live delivery is unaffected by buffer size regardless). Also bumps to RxSharp 0.1.3 (pending publish - see hardkoded/ReactiveExtensions-Sharp#2), which renames the consolidated Extras class to RxExtensions and documents this exact bufferSize risk for future callers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K92eapPm8e7mX7puT4cF4s
kblok
commented
Jul 28, 2026
RxSharp 0.1.4 (pending publish - see hardkoded/ReactiveExtensions-Sharp#3) makes FromEventBuffered's bufferSize nullable, defaulting to ReplaySubject's own unbounded default instead of an opinionated 1. That's the same guarantee EventBufferSize = 16 was working around locally, so it's redundant now - the safe behavior is just the library default. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K92eapPm8e7mX7puT4cF4s
The library's namespace now matches what we've been installing all along (ReactiveExtensionsSharp - see hardkoded/ReactiveExtensions-Sharp#4 for why). Purely mechanical: using RxSharp* -> using ReactiveExtensionsSharp* in the three files that reference it, plus the package reference bump. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K92eapPm8e7mX7puT4cF4s
Purely a naming aid in the using block itself - `Observable<T>`, `Unit`, and every Map/Filter/RaceWithSignalAndTimer call stay unqualified exactly as before, since the plain `using ReactiveExtensionsSharp;` import is still there doing that work. The alias adds no new qualified references anywhere in the file bodies. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K92eapPm8e7mX7puT4cF4s
RaceWithSignalAndTimer used to fake timeout/cancellation into Observable<T> branches via AssumeNeverEmits just to sit in an Observable-level RaceWith next to real data. Racing at the Task level instead (ReactiveExtensionsSharp 0.3.0) sidesteps the problem entirely, and drops a couple of .FirstValueFrom() calls that are no longer needed now that the combinator returns Task<T> directly. Also moved WaitForRequestAsync/WaitForResponseAsync/WaitForFrameAsync/ WaitForNetworkIdleAsync from being duplicated per-protocol in CdpPage/BidiPage into a single shared implementation on the abstract Page class, matching how upstream's Page.ts does it. Comparing our port against upstream surfaced a few real gaps this fixes as a side effect: WaitForOptions.CancellationToken was silently ignored everywhere; we raced against an internal session-closed task instead of the public Close event; CDP's WaitForNetworkIdleAsync ignored the Concurrency option entirely; WaitForFrameAsync under CDP never checked for an already-matching frame before waiting. In-flight request tracking now lives for the page's whole lifetime (wired once in the constructor, mirroring upstream's own #inflight$) instead of being rebuilt fresh on every WaitForNetworkIdleAsync call, so it correctly reflects requests already in flight before the call.
This was referenced Aug 14, 2026
This was referenced Aug 17, 2026
sondresjolyst
pushed a commit
to sondresjolyst/garge-api
that referenced
this pull request
Aug 24, 2026
Updated [PuppeteerSharp](https://github.com/hardkoded/puppeteer-sharp) from 25.5.0 to 25.7.0. <details> <summary>Release notes</summary> _Sourced from [PuppeteerSharp's releases](https://github.com/hardkoded/puppeteer-sharp/releases)._ ## 25.7.0 ## What's Changed * fix: roll Firefox to 153.0.4 by @kblok in hardkoded/puppeteer-sharp#3554 * docs: clarify CdpHttpRequest owns its logger (upstream #15338 N/A) by @kblok in hardkoded/puppeteer-sharp#3552 * feat(tracing): support BufferSize option in Tracing.StartAsync by @kblok in hardkoded/puppeteer-sharp#3551 * refactor: track CDP listeners with DisposableActionsStack by @kblok in hardkoded/puppeteer-sharp#3555 * feat: roll Chrome to 152.0.7977.42 by @kblok in hardkoded/puppeteer-sharp#3553 * Bump version to 25.7.0 by @kblok in hardkoded/puppeteer-sharp#3556 **Full Changelog**: hardkoded/puppeteer-sharp@v25.6.0...v25.7.0 ## 25.6.0 ## What's Changed * Replace hand-rolled retry/timeout plumbing with RxSharp by @kblok in hardkoded/puppeteer-sharp#3520 * Roll browsers: Chrome 151.0.7922.77, Firefox 153.0.3 by @kblok in hardkoded/puppeteer-sharp#3546 * Propagate ILoggerFactory through BiDi sessions, frames and realms by @kblok in hardkoded/puppeteer-sharp#3547 * Default BidiRealm logger to NullLogger by @kblok in hardkoded/puppeteer-sharp#3550 * Don't fail per-frame CDP fan-out when an OOP iframe goes away by @kblok in hardkoded/puppeteer-sharp#3548 **Full Changelog**: hardkoded/puppeteer-sharp@v25.5.0...v25.6.0 Commits viewable in [compare view](hardkoded/puppeteer-sharp@v25.5.0...v25.7.0). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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
2 participants
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.
Upstream Puppeteer solves the "retry an action" and "wait for an event, racing a timeout and session-close" problems with rxjs (
firstValueFrom+raceWithoverfromEmitterEventstreams). We were reimplementing each of these by hand in C#, per method —Locator.RunWithRetryAsyncwas a manual retry loop with a linkedCancellationTokenSourceand a pile of catch clauses just to tell timeout apart from cancellation apart from "retry again";WaitForNetworkIdleAsync,WaitForRequestAsync,WaitForResponseAsync, andWaitForFrameAsynceach hand-rolled their ownTaskCompletionSource+ manual event teardown + timeout shape, once per protocol.All of it now goes through ReactiveExtensionsSharp, a faithful RxJS port built for exactly this kind of swap.
Locatoruses itsRetryAndRaceWithSignalAndTimercombinator; the fourWait*methods above (plusBrowser.WaitForTargetAsync) read close to upstream's own shape — merge the live events with the existing-item snapshot, filter, race against timeout/cancellation/close.A bigger change landed along the way:
WaitForRequestAsync/WaitForResponseAsync/WaitForFrameAsync/WaitForNetworkIdleAsyncused to beabstract, duplicated separately inCdpPageandBidiPage. Upstream implements these once, in the shared abstractPageclass, since they only need generic event-emitter access. Comparing our port against upstream's actual code surfaced real gaps from that duplication, all fixed by unifying:WaitForOptions.CancellationTokenwas silently ignored by all four methods on both protocols, despite being a documented, public option.Closeevent, which is what upstream actually races against.WaitForNetworkIdleAsyncignored theConcurrencyoption entirely — only Bidi's separate implementation honored it.WaitForFrameAsyncnever checked for an already-matching frame before waiting; only Bidi did.In-flight request tracking for
WaitForNetworkIdleAsyncnow lives for the page's whole lifetime (wired once in the constructor) instead of being rebuilt fresh on every call, mirroring upstream's own#inflight$— a fresh-per-call counter would miss requests that were already in flight before the call.Two real bugs also came up mid-port, both from .NET event delivery being able to run on another thread (unlike upstream's single-threaded JS, where the equivalent race can't exist): a matching event firing in the gap between attaching a handler and this code actually subscribing to the Rx pipeline. Fixed with
FromEventBuffered, a library primitive that attaches immediately and buffers into aReplaySubject— reusable instead of hand-rolled per call site.Same external behavior as before everywhere else — same exception types/messages, same timing semantics.
ScreenRecorderwas deliberately left out of this pass; itsChannels-based design doesn't share the shape Rx solves here.Test plan
LocatorTests,WaitForNetworkIdleTests,WaitForRequestTests,WaitForResponseTests,WaitForFrameTests,BrowserWaitForTargetTests, andCloseTestspass, repeatedly, under both Chrome/CDP and Firefox/BiDinetstandard2.0/net8.0/net10.0, both the default andCDP_ONLYconfigs🤖 Generated with Claude Code
https://claude.ai/code/session_01K92eapPm8e7mX7puT4cF4s