EW-10817 Add WebSocketDataMessage helper for text-or-binary payloads#6739
Open
harrishancock wants to merge 2 commits into
Open
EW-10817 Add WebSocketDataMessage helper for text-or-binary payloads#6739harrishancock wants to merge 2 commits into
harrishancock wants to merge 2 commits into
Conversation
Adds a kj_test suite covering normal HibernationManager behavior (basic comm, close, binary, auto-response, multi-IR, multi-WS, hibernation flows, output gate interactions) along with regression tests for EW-10817 that use KJ_EXPECT_LOG to capture the production "another message send is already in progress" assertion. The regression tests pass while the bug exists and fail loudly when the fix lands. Code comments document each test's purpose, lifecycle, and known workarounds. Supporting TestFixture additions: SetupParams::actorLoopback so the actor and the HibernationManager share one Loopback; newIoContext() and newIncomingRequest(IoContext&) for multiple IRs per IoContext; drainAndDestroy() and pollEventLoop() helpers; getActor() and getTimerChannel() accessors so tests can construct the HM outside any IoContext; resetActor() to simulate eviction by reconstructing the Worker::Actor with the same id and Loopback. Two workarounds for known bugs are noted at their use sites: a leaked api::WebSocket ref to dodge the AsyncObject destructor issue, and an explicit end1->receive() drain in some EW-10817 repros to consume the orphaned BlockedSend at teardown. Both go away once EW-10817 is fixed. Assisted-by: OpenCode:claude-opus-4.7
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Clean wrapper — nice owned/borrowed split and thorough tests.
One medium-severity item: several methods on WebSocketDataMessage return non-owning views into *this's storage and should carry KJ_LIFETIMEBOUND so the compiler can warn on dangling use. The codebase already uses this annotation for the same pattern (e.g., blob.h getData() / getType()).
Contributor
|
I'm Bonk, and I've done a quick review of your PR. Summary: Adds Issues found (1):
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## harris/EW-10817-hibernation-manager-tests #6739 +/- ##
=============================================================================
- Coverage 66.76% 66.75% -0.02%
=============================================================================
Files 403 405 +2
Lines 116449 116788 +339
Branches 19496 19615 +119
=============================================================================
+ Hits 77750 77961 +211
- Misses 27026 27052 +26
- Partials 11673 11775 +102 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Owned text-or-binary message wrapper (and a non-owning Ptr counterpart) for the WebSocket auto-response and outgoing-message paths. Wraps `kj::OneOf<kj::String, kj::Array<byte>>` (and the pointer-shaped equivalent) with a domain-named, read-only inspection surface — `isText` / `isBinary` / `tryGetText` / `tryGetBinary` — plus `size()`, `sendVia(kj::WebSocket&)`, `asPtr()` / `toOwned()`, `asOneOf()` for KJ_SWITCH_ONEOF dispatch, and `operator==(kj::StringPtr)` / `operator==(kj::ArrayPtr<const byte>)` for content-equality checks at matching sites. Inspection on `WebSocketDataMessage` delegates through a temporary `WebSocketDataMessagePtr`, so `KJ_SWITCH_ONEOF(msg.asOneOf())` cases bind to `kj::StringPtr` / `kj::ArrayPtr<const byte>` (not the owned types), and `tryGetText/Binary` return `Maybe<StringPtr/ArrayPtr<const byte>>`. This makes inspection implicitly read-only (no path to obtain a mutable reference into the owned storage) and hides the storage type from callers — owned-vs-borrowed is now purely a lifetime distinction. This pattern — text-or-binary WebSocket payloads, never control frames — recurs across the codebase, and dealing with the equivalent `kj::OneOf` at every consuming site is awkward. The helper consolidates it into a single typed wrapper. Assisted-by: OpenCode:claude-opus-4.7
harrishancock
force-pushed
the
harris/EW-10817-websocket-data-message
branch
from
d4878e5 to
09a0cce
Compare
harrishancock
force-pushed
the
harris/EW-10817-hibernation-manager-tests
branch
2 times, most recently
from
b966caa to
9b82143
Compare
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.
Owned text-or-binary message wrapper (and a non-owning Ptr counterpart) for the WebSocket auto-response and outgoing-message paths. Wraps
kj::OneOf<kj::String, kj::Array<byte>>(and the pointer-shaped equivalent) with a domain-named, read-only inspection surface —isText/isBinary/tryGetText/tryGetBinary— plussize(),sendVia(kj::WebSocket&),asPtr()/toOwned(),asOneOf()for KJ_SWITCH_ONEOF dispatch, andoperator==(kj::StringPtr)/operator==(kj::ArrayPtr<const byte>)for content-equality checks at matching sites.Inspection on
WebSocketDataMessagedelegates through a temporaryWebSocketDataMessagePtr, soKJ_SWITCH_ONEOF(msg.asOneOf())cases bind tokj::StringPtr/kj::ArrayPtr<const byte>(not the owned types), andtryGetText/BinaryreturnMaybe<StringPtr/ArrayPtr<const byte>>. This makes inspection implicitly read-only (no path to obtain a mutable reference into the owned storage) and hides the storage type from callers — owned-vs-borrowed is now purely a lifetime distinction.This pattern — text-or-binary WebSocket payloads, never control frames — recurs across the codebase, and dealing with the equivalent
kj::OneOfat every consuming site is awkward. The helper consolidates it into a single typed wrapper.Assisted-by: OpenCode:claude-opus-4.7