Rename Extensions to RxExtensions, clarify FromEventBuffered's bufferSize risk - #2
Merged
Conversation
…Size risk Extensions read too generically for a class this specific to RxSharp's own combinators. RxExtensions in the existing RxSharp.Extras namespace keeps the same one-name-for-the- whole-surface benefit without the genericness. Also expanded the bufferSize doc comment on FromEventBuffered: the default of 1 is a real footgun for a caller filtering for a specific match among payloads that could plausibly arrive in the pre-subscribe gap this method exists to cover - a matching payload can be silently evicted by a later non-matching one before anyone subscribes. Found via a puppeteer-sharp code review on the two call sites doing exactly that (WaitForTargetAsync/WaitForFrameAsync), which are being fixed to pass a larger bufferSize as a follow-up in that repo. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K92eapPm8e7mX7puT4cF4s
kblok
added a commit
to hardkoded/puppeteer-sharp
that referenced
this pull request
Jul 27, 2026
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
added a commit
to hardkoded/puppeteer-sharp
that referenced
this pull request
Aug 10, 2026
* Replace hand-rolled retry/timeout plumbing in Locator and WaitForNetworkIdle 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 * Replace hand-rolled retry/timeout plumbing in WaitForRequest/Response/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 * Rewrite WaitForTargetAsync/WaitForFrameAsync closer to upstream's shape 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 * Update call sites for RxSharp's consolidated PuppeteerExtras class 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 * Update call sites for RxSharp's Extensions class rename PuppeteerExtras -> Extensions, see hardkoded/ReactiveExtensions-Sharp#1. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K92eapPm8e7mX7puT4cF4s * Fix a real event-loss race in WaitForTargetAsync/WaitForFrameAsync 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 * Apply suggestion from @kblok * Drop EventBufferSize now that RxSharp defaults to unbounded 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 * Update to RxSharp 0.2.0's ReactiveExtensionsSharp namespace 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 * Add a RxSharp using alias for ReactiveExtensionsSharp 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 * Drop AssumeNeverEmits, unify Wait* methods into the shared Page class 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. * Bump version to 25.5.1 --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.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
1 participant
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.
Two independent changes, bundled since both landed the same day.
Rename:
Extensions→RxExtensions. Same class, same purpose (the single-name consolidation of all the Extras static classes from #1), just a less generic name.bufferSize clarification: a puppeteer-sharp code review caught that
FromEventBuffered's defaultbufferSize: 1is a real footgun — if a caller is filtering for a specific match among payloads that could plausibly arrive in the pre-subscribe gap this method exists to cover, a matching payload can be silently evicted from the size-1 buffer by a later non-matching one before anyone subscribes to see it. Not a bug inFromEventBuffereditself (1 is a perfectly reasonable default for "I only care about the most recent value"), but worth spelling out loudly in the doc comment so the next caller doesn't hit it blind. puppeteer-sharp's own two call sites (WaitForTargetAsync/WaitForFrameAsync) are being fixed in a follow-up there to pass a largerbufferSize.All 839 tests still pass.
🤖 Generated with Claude Code
https://claude.ai/code/session_01K92eapPm8e7mX7puT4cF4s