Code-quality review: correctness fixes, ErrRequestOverloaded, removeElementsLocked, file split#80
Merged
Merged
Conversation
Behavioral and structural fixes from the core-package quality review; each comes with a test. No public API removals. - handleBroadcast: reject an HTML-id broadcast target containing a tab or newline before it is written verbatim into a wire frame (Jid<0), where it would corrupt framing. A valid HTML id never contains ASCII whitespace, so reject via reportMisuse rather than escape, satisfying wire.WsMsg.Append's documented no-tab/newline precondition. - /jaws/.tail/<key>: bind the one-shot tail drain to the client IP with the same loopback-aware equalIP check the WebSocket claim path uses, so a leaked key cannot drain (and thereby deny) another client's tail. A mismatch is treated as not found. - Add the exported ErrRequestOverloaded sentinel and wrap the broadcast- and event-channel backpressure cancellation causes with it, so callers can match the teardown reason via context.Cause + errors.Is instead of string-matching. - Extract removeElementsLocked, shared by handleRemove and deleteElementLocked, removing a byte-for-byte duplicated tagMap-cleanup loop. - GenerateHeadHTML: deduplicate extra resources against the built-in CSS path as well as the JS path, so re-listing a built-in is a no-op for both. - Add a regression test that errors.Is(err, ErrEventHandlerPanic) stays panic-safe when the handler panicked with a non-comparable value.
Pure code-motion, no behavior change: build, go vet, staticcheck, golangci-lint and the full -race / -tags debug -race suites are unchanged. - Move the Jaws-side session helpers (SessionCount, Sessions, getSessionLocked, getCookieSessionsIDs, GetSession, NewSession, newSession, deleteSession) and the session middleware (sessioner, Jaws.Session) into session.go, which already owns the Session methods. - Move the broadcast/dirt/redirect/alert machinery and the SetInner..JsCall convenience wrappers into a new broadcast.go. jaws.go shrinks from 1395 to 936 lines and now holds the instance lifecycle, request pool, Serve loop, client-IP helpers and HTTP routing.
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.
Code changes from a quality review of the core
jawspackage. Documentation-onlyimprovements are split into a separate stacked PR (
code-quality-review-docs,based on this branch) so this PR stays focused on behavior and structure.
The review found no high/critical issues; these are the low-severity items worth
fixing, each with a test, plus one mechanical refactor.
Fixes (commit 1)
a tab or newline before it is written verbatim into a wire frame (
Jid<0), where itwould corrupt framing. Satisfies
wire.WsMsg.Append's documented no-tab/newlineprecondition; rejects (not escapes) since a valid HTML id has no ASCII whitespace.
with the same loopback-aware
equalIPcheck the WebSocket claim path uses, so aleaked key cannot drain (and thereby deny) another client's tail.
backpressure cancellation causes now wrap it, so callers can classify the teardown
via
context.Cause+errors.Isinstead of string-matching.tagMap-cleanup loopshared by
handleRemoveanddeleteElementLocked.well as the JS path (previously only the JS path was guarded).
errors.Is(err, ErrEventHandlerPanic)stays panic-safe when ahandler panics with a non-comparable value.
Refactor (commit 2)
session.go, and the broadcast/dirt/redirect/alert +SetInner..JsCallhelpers intoa new
broadcast.go.jaws.goshrinks 1395 → 936 lines. No behavior change.Verification
go build,go vet,staticcheck,golangci-lintall clean;go test -race .andgo test -tags debug -race .both pass.Note: one reviewed finding (a theoretical
errors.Ispanic via the customIsmethods) was investigated and not changed — the panic is unreachable through the
public API (unexported sentinel types; the exported sentinels carry nil interface
fields) and the proposed fix would not address the actual stdlib comparison site. A
regression test documents the reachable safe behavior instead.