diff --git a/STATUS.md b/STATUS.md index 6ef3b075..d12262f3 100644 --- a/STATUS.md +++ b/STATUS.md @@ -107,7 +107,7 @@ By package, bottom-up along the dependency stack: | § | Feature | Status | Notes | |---------|----------------------------------|--------|-------| -| 5.1 | Subscriptions | DONE | Subscribe/Publish/OK/Error state machine in `pubsub.go`. | +| 5.1 | Subscriptions | DONE | Subscribe/Publish/OK/Error state machine in `subscribe.go` and `publish.go`. A second response to this side's SUBSCRIBE or PUBLISH closes the session with PROTOCOL_VIOLATION: on a `Subscription`'s or `Session.Publish` `Publication`'s broker, a SUBSCRIBE_OK, or a REQUEST_OK / REQUEST_ERROR before any Update (after one it may answer an Update that gave up); in the relay, a REQUEST_OK / REQUEST_ERROR on a forwarded PUBLISH. | | 5.1.1 | Subscription state management | DONE | REQUEST_ERROR / STOP_SENDING / PUBLISH_DONE handling + cleanup. The relay resets a cancelled subscription's open subgroup and fill streams. | | 5.1.2 | Location filters | DONE | Every start/end form (unfiltered, Next Object, relative and absolute start, absolute range) + `Matches`. | | 5.1.3 | Fill semantics | PARTIAL | Fill fetch streams from FILL_PARAMETERS on SUBSCRIBE / REQUEST_UPDATE (`handler_fill.go`), and on SUBSCRIBE_TRACKS, one per forwarded PUBLISH's subscription, keyed to the PUBLISH's Request ID (§10.1). A fill inherits the subscription's Range Filters; the ones inside FILL_PARAMETERS override per type. A cancelled subscription's open fills are reset (§5.1.3.1). Not done: scheduling fills against their subscription (§7.2, see Limitations). | @@ -166,8 +166,8 @@ By package, bottom-up along the dependency stack: | 10.2.5 | FILL_TIMEOUT | 0x0A | DONE | The budget for a FETCH's or fill's upstream FETCH, its response included: when it runs out, what arrived is served and the rest is an End of Timed-Out Range; 0 asks no upstream. Default 5s. | | 10.2.6 | RENDEZVOUS_TIMEOUT | 0x04 | DONE | | | 10.2.7 | SUBSCRIBER_PRIORITY | 0x20 | DONE | | -| 10.2.8 | GROUP_ORDER | 0x22 | DONE | Ascending/Descending validated. | -| 10.2.9 | LOCATION_FILTER | 0x21 | DONE | Overflow-checked. | +| 10.2.8 | GROUP_ORDER | 0x22 | DONE | A value outside {1, 2} closes the session, wherever it appears, FILL_PARAMETERS included. | +| 10.2.9 | LOCATION_FILTER | 0x21 | DONE | An end Group overflowing 2^64-1 closes the session with PROTOCOL_VIOLATION (§5.1.2); a value that does not parse, with KEY_VALUE_FORMATTING_ERROR (§1.4.3). | | 10.2.10 | SUBGROUP_FILTER | 0x25 | DONE | Enforced per object in the fanout/FETCH. | | 10.2.11 | OBJECTID_FILTER | 0x26 | DONE | Enforced per object in the fanout/FETCH. | | 10.2.12 | PRIORITY_FILTER | 0x27 | DONE | Enforced per object (subgroup priority); >255 rejected INVALID_FILTER. | @@ -176,7 +176,7 @@ By package, bottom-up along the dependency stack: | 10.2.15 | FILL_PARAMETERS | 0x23 | PARTIAL| Inner Table 6 scope and duplicates checked; omitted Range Filters are not inherited from the subscription (see §5.1.3). | | 10.2.16 | EXPIRES | 0x08 | DONE | | | 10.2.17 | LARGEST_OBJECT | 0x09 | DONE | Monotonic constraint applied. | -| 10.2.18 | FORWARD | 0x10 | DONE | | +| 10.2.18 | FORWARD | 0x10 | DONE | A value above 1 closes the session, in every message that may carry it. | | 10.2.19 | NEW_GROUP_REQUEST | 0x32 | DONE | | | 10.2.20 | TRACK_NAMESPACE_PREFIX | 0x34 | DONE | Applied on REQUEST_UPDATE; SUBSCRIBE_NAMESPACE reconciles its announced set. | | 10.2.21 | INCLUDE_PROPERTIES | 0x35 | DONE | A value other than 0 or 1 closes the session. With 0 the relay sends empty Track Properties in SUBSCRIBE_OK, FETCH_OK, TRACK_STATUS_OK and forwarded PUBLISH, and writes the priority inline on that subscription's subgroups and datagrams, since the subscriber cannot inherit DEFAULT_PUBLISHER_PRIORITY. | @@ -188,9 +188,9 @@ By package, bottom-up along the dependency stack: | 10.3.1.5| MOQT_IMPLEMENTATION | 0x07 | DONE | Advisory. | | 10.3.1.6| MAX_FILTER_RANGES | 0x06 | DONE | `WithMaxFilterRanges` advertises it; relay rejects over-limit/prohibited filters with INVALID_FILTER. The relay advertises `relay.DefaultMaxFilterRanges` (16) rather than inheriting the session default of 0, which would prohibit the Range Filters it fully implements; `relay.Config.MaxFilterRanges` overrides, negative to prohibit. | | 10.3.1.7| MAX_REQUEST_UPDATES | 0x08 | DONE | `WithMaxRequestUpdates` advertises the per-stream limit; enforced on inbound follow-ups via `RequestUpdateLimiter`, closing with `TOO_MANY_REQUEST_UPDATES` on overflow. | -| 10.4 | GOAWAY | 0x10 | DONE | Same encoding on control and request streams (draft-19 dropped the Request ID field); callback. As recipient the relay initiates no new SUBSCRIBE, FETCH or PUBLISH to the peer and leaves closing the session to the sender. | +| 10.4 | GOAWAY | 0x10 | DONE | Same encoding on control and request streams (draft-19 dropped the Request ID field); callback. As recipient the relay initiates no new SUBSCRIBE, FETCH or PUBLISH to the peer and leaves closing the session to the sender. On a request stream (`RequestBroker.Serve` and the relay's readers after the response; `session.RequestGoaways` for callers that read one themselves) a second GOAWAY, or one with a New Session URI received by a server, closes the session with PROTOCOL_VIOLATION; a single one is handed to the reader, and neither side migrates the request. | | 10.5 | REQUEST_OK | 0x07 | DONE | Shared OK for PUBLISH/UPDATE/TRACK_STATUS/namespace reqs. Track Properties where they must be empty close the session on receipt and are refused on send (`ErrTrackPropertiesNotAllowed`). | -| 10.6 | REQUEST_ERROR (+ Redirect) | 0x05 | DONE | Redirect required only when code==REDIRECT. `Request.Reject` sends a Retry Interval; the relay invites a jittered ~1 s retry on EXCESSIVE_LOAD and passes an upstream SUBSCRIBE rejection on by meaning, Retry Interval kept. | +| 10.6 | REQUEST_ERROR (+ Redirect) | 0x05 | DONE | Redirect required only when code==REDIRECT, and exposed as `RequestRejectedError.Redirect`; `Request.Reject` sends one. A Connect URI received by a server, or a Track Name for SUBSCRIBE_NAMESPACE / PUBLISH_NAMESPACE / SUBSCRIBE_TRACKS, closes the session (§10.6.1), and `Reject` refuses to send either; on a REQUEST_UPDATE's answer only the Connect URI is checked, as the reader does not know the request, and an update handler's REDIRECT is sent as INTERNAL_ERROR (§10.6.2 does not list REQUEST_UPDATE). The relay does not follow a Redirect: an upstream REDIRECT becomes INTERNAL_ERROR downstream. `Request.Reject` sends a Retry Interval; the relay invites a jittered ~1 s retry on EXCESSIVE_LOAD and passes an upstream SUBSCRIBE rejection on by meaning, Retry Interval kept. | | 10.7 | SUBSCRIBE | 0x03 | DONE | | | 10.8 | SUBSCRIBE_OK | 0x04 | DONE | Registers inbound track alias. | | 10.9 | REQUEST_UPDATE | 0x02 | DONE | A REQUEST_UPDATE opening a request stream closes the session with PROTOCOL_VIOLATION (`ErrUnexpectedRequestUpdate`). | @@ -203,15 +203,15 @@ By package, bottom-up along the dependency stack: | 10.16 | PUBLISH_NAMESPACE | 0x06 | DONE | | | 10.17 | NAMESPACE | 0x08 | DONE | Per namespace, counted over local and remote sources. | | 10.18 | NAMESPACE_DONE | 0x0E | DONE | Never before its NAMESPACE. | -| 10.19 | SUBSCRIBE_NAMESPACE | 0x50 | DONE | | -| 10.20 | SUBSCRIBE_TRACKS | 0x51 | DONE | §10.20.1: its SUBSCRIBE parameters become each forwarded PUBLISH's subscription; an out-of-range value closes the session (§10.2.8/§10.2.18). A REQUEST_UPDATE merges into them for later PUBLISHes (§10.2.18: "Existing subscriptions are unaffected"), and existing tracks that newly match by prefix or Range Filter are forwarded then. | -| 10.21 | PUBLISH_SKIPPED | 0x0F | DONE | Prohibition scoped to a single PUBLISH (§6.1) — not sticky across re-PUBLISHes. | +| 10.19 | SUBSCRIBE_NAMESPACE | 0x50 | DONE | A first response other than REQUEST_OK / REQUEST_ERROR, a GOAWAY included, closes the session with PROTOCOL_VIOLATION. `NamespaceSubscription.Broker` closes it on a NAMESPACE_DONE for a suffix no NAMESPACE announced, and on a PUBLISH_STATE_NOTIFY or REQUEST_UPDATE from the publisher (§10.9, §10.10); `NamespaceSubscription.Update` updates the subscription through the broker. The NAMESPACE_DONE check resolves suffixes against the prefix in force, switching at the REQUEST_OK that accepts a TRACK_NAMESPACE_PREFIX update (§10.9.2); once any Update on the subscription gives up, responses no longer pair reliably, so the prefix is unknown and the check stops. A caller reading the stream with `message.Parse` gets none of these checks. | +| 10.20 | SUBSCRIBE_TRACKS | 0x51 | DONE | A first response other than REQUEST_OK / REQUEST_ERROR, a GOAWAY included, closes the session with PROTOCOL_VIOLATION. §10.20.1: its SUBSCRIBE parameters become each forwarded PUBLISH's subscription; an out-of-range value closes the session (§10.2.8/§10.2.18). A REQUEST_UPDATE merges into them for later PUBLISHes (§10.2.18: "Existing subscriptions are unaffected"), and existing tracks that newly match by prefix or Range Filter are forwarded then. | +| 10.21 | PUBLISH_SKIPPED | 0x0F | DONE | Prohibition scoped to a single PUBLISH (§6.1) — not sticky across re-PUBLISHes. `TrackSubscription.ReadPublishSkipped` and its broker close the session on a PUBLISH_STATE_NOTIFY or REQUEST_UPDATE from the publisher (§10.9, §10.10); `ReadPublishSkipped` skips a single GOAWAY (§10.4). | ## §11 Data streams and datagrams | § | Feature | Status | Notes | |----------|--------------------------------------|---------|-------| -| 11.1 | Track alias | DONE | In subgroup header + datagram; validated. | +| 11.1 | Track alias | DONE | In subgroup header + datagram; validated. A SUBSCRIBE_OK or PUBLISH reusing the alias of a different registered Track closes the session with DUPLICATE_TRACK_ALIAS (`Session.RegisterInboundTrack`). A `Subscription` or `IncomingPublication` releases its alias once Terminated (§5.1): on its own or its broker's `Close`, when the publisher's FIN is read through it or its broker's `Serve`, or when `Serve` cancels the stream. | | 11.2 | Objects / object header | DONE | All header fields encoded. | | 11.2.1.1 | Object status | DONE | Normal / EndOfGroup / EndOfTrack. | | 11.2.1.2 | Object properties | DONE | Length-prefixed KV pairs. | @@ -233,8 +233,8 @@ By package, bottom-up along the dependency stack: | 12.2 | OBJECT_DELIVERY_TIMEOUT | 0x02 | DONE | Track + Object Property; first-object override, as §12.1. | | 12.3 | MAX_CACHE_DURATION | 0x04 | DONE | Per Object: each carries the value of the upstream it arrived through (captured with the Track Alias in `session.InboundTrack`), and is not forwarded live or served from the cache past it; a present 0 is never served from the cache. In FETCH and fill an expired Object is an End of Unknown Range, whether it expired before the snapshot or while the stream was written. | | 12.4 | DEFAULT_PUBLISHER_PRIORITY | 0x0E | DONE | | -| 12.5 | DEFAULT_PUBLISHER_GROUP_ORDER | 0x22 | DONE | Validated. | -| 12.6 | DYNAMIC_GROUPS | 0x30 | DONE | Property defined & scope-validated (flow: see §5.1.6.1). | +| 12.5 | DEFAULT_PUBLISHER_GROUP_ORDER | 0x22 | DONE | A value outside {1, 2} closes the session, also inside Immutable Properties; an omitted one is Ascending. | +| 12.6 | DYNAMIC_GROUPS | 0x30 | DONE | A value above 1 closes the session, also inside Immutable Properties. | | 12.7 | Immutable properties | 0x0B | DONE | Relays cache & forward verbatim, never add. Property lookups search its contents too (`message.ExpandImmutable`), the mutable value winning: delivery timeouts, MAX_CACHE_DURATION, DYNAMIC_GROUPS, Mandatory Track Property screening, and property Range Filters. | | 12.8 | Prior group ID gap | 0x3C | PARTIAL| Object-scope; encoder in `msf/groupid.go`. More than one, or one past the Group ID, makes the track malformed (`message.CheckObjectProperties`), and the relay also ends the track for two values in one Group. Against the last 32 Groups of any upstream (`registry.TrackEntry.ClaimDelivered`), the relay neither forwards nor caches an Object in a Group announced absent; a gap covering a received Group is accepted (§2.1, §9.1, see Limitations). Not in upstream FETCH responses, nor in a session that is not a relay's. | | 12.9 | Prior object ID gap | 0x3E | PARTIAL| Object-scope. More than one, or one past the Object ID, makes the track malformed. The relay neither forwards nor caches an Object announced absent, and accepts a gap covering a received Object, as for §12.8. | @@ -333,10 +333,6 @@ Known protocol gaps, roughly ordered by how load-bearing they are: self-limit *outbound* REQUEST_UPDATEs against a peer's advertised value for the same reason: `UpdateRequest`/`RequestBroker.Update` are synchronous write-then-read, so they never exceed any limit ≥ 1. -- **Out-of-range GROUP_ORDER on FETCH (§10.2.8)** — the SUBSCRIBE and - SUBSCRIBE_TRACKS paths now close the session with PROTOCOL_VIOLATION on an - out-of-range GROUP_ORDER/FORWARD (§10.2.8/§10.2.18), but the FETCH path - still reads a bad GROUP_ORDER as Ascending pending the same promotion. - **Late publisher pickup (§9.5)** — multiple publishers per track are merged and deduplicated, and a local PUBLISH_NAMESPACE that arrives after a track's upstream set is established is SUBSCRIBEd for each matching track. Two @@ -522,27 +518,21 @@ already listed as Limitations above are not repeated here. Session layer: -- DUPLICATE_TRACK_ALIAS never closes the session: the relay answers REQUEST_ERROR - MALFORMED_TRACK, `Session.Subscribe` and `AcceptPublish` return an error (§11.1). - A session-layer subscriber also never releases an alias when its subscription - ends. -- A GOAWAY on a request stream is ignored: a second one, or one carrying a New - Session URI sent to a server, does not close the session (§10.4). -- A REQUEST_ERROR Redirect is dropped after parsing: a server receiving a Connect - URI, or a Track Name on a namespace-scoped request, does not close the session, - and the application cannot follow it (§10.6.1). -- A first response other than REQUEST_OK / REQUEST_ERROR to SUBSCRIBE_NAMESPACE or - SUBSCRIBE_TRACKS does not close the session (§10.19, §10.20). +- On a request stream the relay answered, a REQUEST_OK or REQUEST_ERROR from + the requester is ignored rather than closing the session, so a Connect URI in + one is not checked (§10.6.1). The draft defines no such message; only on a + forwarded PUBLISH, where it is a second response, does the relay close the + session (§5.1). +- A GOAWAY on a request stream before its initial response (other than to + SUBSCRIBE_NAMESPACE or SUBSCRIBE_TRACKS, where §10.19/§10.20 make it a + PROTOCOL_VIOLATION) is an error rather than a legal message checked + against §10.4. - `Publication`'s automatic PUBLISH_DONE UPDATE_FAILED is sent while its subgroup streams are open, `WriteObject` still succeeds after `Done`, and a subgroup opened concurrently with `Done` is missing from the Stream Count (§10.12). - `Publication`'s REQUEST_UPDATE_OK carries LARGEST_OBJECT only for Objects it wrote itself, not the one its SUBSCRIBE_OK or PUBLISH reported (§10.2.17, §10.9.1). -- `ReadPublishSkipped` does not close the session on a REQUEST_UPDATE or - PUBLISH_STATE_NOTIFY from the publisher (§10.9, §10.10), and nothing enforces - NAMESPACE_DONE-before-NAMESPACE on a namespace subscription (§10.19). -- A second SUBSCRIBE_OK is handed to the application (§5.1 SHOULD close). - A rejected request sends STOP_SENDING with INTERNAL_ERROR (§3.3.4 SHOULD use a relevant code). - Mandatory Track Property enforcement is off unless configured (§2.5.1). @@ -551,18 +541,6 @@ Session layer: - SETUP options are sorted unstably, so with more than 12 the Token order on the wire can differ from the order `heldSetupAliases` replays (§10.3.1.4). -Validation (values that MUST close the session): - -- GROUP_ORDER on PUBLISH, inside FILL_PARAMETERS, and in `AcceptSubscribe` / - `AcceptPublish` (§10.2.8); the FETCH case is the Limitation above. -- FORWARD in PUBLISH_STATE_NOTIFY, in a publisher's REQUEST_UPDATE, and in - `AcceptPublish` (§10.2.18). -- DEFAULT_PUBLISHER_GROUP_ORDER outside {1, 2} and DYNAMIC_GROUPS above 1 in - Track Properties (§12.5, §12.6). -- A LOCATION_FILTER whose StartGroup + EndGroupDelta overflows: REQUEST_ERROR - MALFORMED_TRACK on SUBSCRIBE, REQUEST_UPDATE and SUBSCRIBE_TRACKS, INVALID_FILTER - on FETCH, a fill reset in FILL_PARAMETERS (§5.1.2). - Relay: - Any REQUEST_UPDATE turns INCLUDE_PROPERTIES=0 back off, so the subscriber @@ -599,13 +577,11 @@ Relay: Documentation: -- Limitations: "Duplicate Objects … are not compared" is stale; the FETCH - GROUP_ORDER entry omits PUBLISH and FILL_PARAMETERS; the LOC entry names +- Limitations: "Duplicate Objects … are not compared" is stale; the LOC entry names `PropAudioLevel = 0x0A` (it is 0x0C); "Handles the application reads itself" says `CheckPeerParams` checks roles; "Inbound GOAWAY" omits request streams. -- Table rows 10.2.6, 10.2.8, 10.2.9, 10.2.15, 10.2.18, 10.2.21, 12.3, - 12.5 and 12.6 overstate what is done (see the items above), and the package - summary still lists joining FETCH. +- Table rows 10.2.6, 10.2.15, 10.2.21 and 12.3 overstate what is done (see + the items above), and the package summary still lists joining FETCH. - `session/namespace.go` says NAMESPACE / NAMESPACE_DONE go on a PUBLISH_NAMESPACE stream (§10.17, §10.18). - About a dozen stale `§` citations (padding, grease, fetch ordering, caching). diff --git a/pkg/moqt/message/fill.go b/pkg/moqt/message/fill.go index 2438d1ad..abe34548 100644 --- a/pkg/moqt/message/fill.go +++ b/pkg/moqt/message/fill.go @@ -1,6 +1,7 @@ package message import ( + "errors" "fmt" "slices" @@ -47,7 +48,13 @@ func FillParametersFromParam(ps Parameters) (inner Parameters, ok bool, err erro return nil, false, nil } if err := inner.parse(wire.NewReader(p.Bytes)); err != nil { - return nil, true, fmt.Errorf("moqt/message: FILL_PARAMETERS: %w", err) + // §10.2: an unknown parameter, or a Type past 2^64-1, is a + // PROTOCOL_VIOLATION here as in any message; anything else is a + // value that does not parse (§1.4.3). + if errors.Is(err, ErrUnknownParameter) || errors.Is(err, errParamTypeOverflow) { + return nil, true, fmt.Errorf("moqt/message: FILL_PARAMETERS: %w", err) + } + return nil, true, fmt.Errorf("%w: FILL_PARAMETERS: %w", ErrValueFormatting, err) } for _, ip := range inner { if !slices.Contains(fillParamsAllowed, ip.Type) { diff --git a/pkg/moqt/message/filter.go b/pkg/moqt/message/filter.go index 86eda102..ba0c6106 100644 --- a/pkg/moqt/message/filter.go +++ b/pkg/moqt/message/filter.go @@ -1,6 +1,7 @@ package message import ( + "errors" "fmt" "math" @@ -78,13 +79,16 @@ func (f *LocationFilter) Validate() error { return fmt.Errorf("moqt/message: LOCATION_FILTER has %d fields, want 0-4 (§5.1.2)", f.Fields) } if f.HasEnd() && f.StartGroup > math.MaxUint64-f.EndGroupDelta { - return fmt.Errorf( - "moqt/message: LOCATION_FILTER end group overflow (start=%d delta=%d) (PROTOCOL_VIOLATION §5.1.2)", - f.StartGroup, f.EndGroupDelta) + return fmt.Errorf("%w (start=%d delta=%d)", errEndGroupOverflow, f.StartGroup, f.EndGroupDelta) } return nil } +// errEndGroupOverflow is a LOCATION_FILTER whose StartGroup + EndGroupDelta +// exceeds 2^64 - 1: a PROTOCOL_VIOLATION (§5.1.2), unlike a value that does +// not parse ([ErrValueFormatting]). +var errEndGroupOverflow = errors.New("moqt/message: LOCATION_FILTER end group overflow (PROTOCOL_VIOLATION §5.1.2)") + // Start resolves the first Location that passes the filter, given the // publisher's current Largest Object. hasLargest is false before anything has // been published on the track, which §5.1.2 pins to {0, 0}. diff --git a/pkg/moqt/message/param_scope.go b/pkg/moqt/message/param_scope.go index 0b47a002..a13f3a75 100644 --- a/pkg/moqt/message/param_scope.go +++ b/pkg/moqt/message/param_scope.go @@ -218,10 +218,18 @@ func (e *ParamScopeError) Error() string { return fmt.Sprintf("moqt/message: %s not allowed in %s (PROTOCOL_VIOLATION §10.2.1)", e.Type, e.Scope) } +// ErrValueFormatting marks a parameter whose value does not match the +// serialization its type defines. §1.4.3: the receiver "MUST close the session +// with error code KEY_VALUE_FORMATTING_ERROR". +var ErrValueFormatting = errors.New("moqt/message: value does not match its type's serialization") + // CheckScope reports the first parameter of ps not allowed in a message of the // given scope (§10.2.1) or repeated where it may not be (§10.2), and validates -// the FILL_PARAMETERS (§10.2.15) and INCLUDE_PROPERTIES (§10.2.21) values. -// Every error is a session-level PROTOCOL_VIOLATION. +// the values a receiver must close the session for, in ps and inside its +// FILL_PARAMETERS (§10.2.15): GROUP_ORDER (§10.2.8), FORWARD (§10.2.18), +// INCLUDE_PROPERTIES (§10.2.21) and LOCATION_FILTER (§5.1.2). Every error is a +// session-level PROTOCOL_VIOLATION, except one wrapping [ErrValueFormatting], +// a KEY_VALUE_FORMATTING_ERROR. func (ps Parameters) CheckScope(scope ParamScope) error { for _, p := range ps { allowed := paramScopes[p.Type] @@ -236,13 +244,35 @@ func (ps Parameters) CheckScope(scope ParamScope) error { if t, dup := ps.firstDuplicate(); dup { return &ParamScopeError{Type: t, Scope: scope, Duplicate: true} } - if _, _, err := FillParametersFromParam(ps); err != nil { + inner, _, err := FillParametersFromParam(ps) + if err != nil { return err } + if err := inner.checkValues(); err != nil { + return fmt.Errorf("FILL_PARAMETERS: %w", err) + } + return ps.checkValues() +} + +// checkValues validates the values in ps a receiver must close the session +// for; see [Parameters.CheckScope]. +func (ps Parameters) checkValues() error { + if p, ok := ps.Find(ParamGroupOrder); ok && + GroupOrder(p.Byte) != GroupOrderAscending && GroupOrder(p.Byte) != GroupOrderDescending { + return fmt.Errorf("moqt/message: GROUP_ORDER value %d outside {1,2} (PROTOCOL_VIOLATION §10.2.8)", p.Byte) + } + if p, ok := ps.Find(ParamForward); ok && p.Byte > 1 { + return fmt.Errorf("moqt/message: FORWARD value %d outside {0,1} (PROTOCOL_VIOLATION §10.2.18)", p.Byte) + } if _, err := IncludePropertiesFromParam(ps); err != nil { return err } - return nil + switch _, err := LocationFilterFromParam(ps); { + case err == nil, errors.Is(err, errEndGroupOverflow): + return err + default: + return fmt.Errorf("%w: LOCATION_FILTER: %w", ErrValueFormatting, err) + } } // firstDuplicate reports the first parameter type repeated in ps where its diff --git a/pkg/moqt/message/params.go b/pkg/moqt/message/params.go index 11211fde..8f66f34d 100644 --- a/pkg/moqt/message/params.go +++ b/pkg/moqt/message/params.go @@ -358,6 +358,10 @@ func (ps Parameters) append(w *wire.Writer) { } } +// errParamTypeOverflow is a parameter Type delta taking the Type past 2^64-1: +// a PROTOCOL_VIOLATION (§10.2), unlike a value that does not parse. +var errParamTypeOverflow = errors.New("moqt/message: parameter type delta overflow") + // parse reads a Number-of-Parameters varint followed by that many parameters // from r. func (ps *Parameters) parse(r *wire.Reader) error { @@ -376,7 +380,7 @@ func (ps *Parameters) parse(r *wire.Reader) error { return err } if delta > ^uint64(0)-prev { - return errors.New("moqt/message: parameter type delta overflow") + return errParamTypeOverflow } t := prev + delta p := Parameter{Type: ParamID(t)} diff --git a/pkg/moqt/message/properties.go b/pkg/moqt/message/properties.go index 4906376e..875b080d 100644 --- a/pkg/moqt/message/properties.go +++ b/pkg/moqt/message/properties.go @@ -195,6 +195,29 @@ func AppendTrackProperties(pairs []wire.KVPair) []byte { return w.Bytes() } +// CheckTrackPropertyValues reports a Track Property in pairs (Immutable +// Properties expanded, see [ExpandImmutable]) whose value the draft makes +// session-fatal: DEFAULT_PUBLISHER_GROUP_ORDER outside {1, 2} (§12.5) or +// DYNAMIC_GROUPS above 1 (§12.6). The caller MUST close the session with +// PROTOCOL_VIOLATION. +func CheckTrackPropertyValues(pairs []wire.KVPair) error { + for _, kv := range pairs { + switch kv.Type { + case PropertyDefaultPublisherGroupOrder: + if kv.IntVal != uint64(GroupOrderAscending) && kv.IntVal != uint64(GroupOrderDescending) { + return fmt.Errorf( + "moqt/message: DEFAULT_PUBLISHER_GROUP_ORDER value %d outside {1,2} (PROTOCOL_VIOLATION §12.5)", + kv.IntVal) + } + case PropertyDynamicGroups: + if kv.IntVal > 1 { + return fmt.Errorf("moqt/message: DYNAMIC_GROUPS value %d above 1 (PROTOCOL_VIOLATION §12.6)", kv.IntVal) + } + } + } + return nil +} + // FirstUnknownMandatoryTrackProperty returns the first Mandatory Track // Property (range 0x4000–0x7FFF) in pairs whose type is not in knownTypes, // and whether one was found — the offending type is what callers need to diff --git a/pkg/moqt/session/broker.go b/pkg/moqt/session/broker.go index 795b41a9..e5eb06bd 100644 --- a/pkg/moqt/session/broker.go +++ b/pkg/moqt/session/broker.go @@ -10,6 +10,8 @@ import ( "github.com/floatdrop/moq-go/pkg/moqt" "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/track" + "github.com/floatdrop/moq-go/pkg/moqt/wire" ) // RequestBroker owns an established request stream's read side and @@ -30,8 +32,16 @@ import ( // [RequestBroker.HandleUpdates], or declined with NOT_SUPPORTED when there // is none, since acknowledging an unapplied update would misstate the // request's state. -// - Everything else (PUBLISH_DONE, unsolicited responses, …) is handed to -// Serve's callback. +// - On the broker of a [Subscription], or of a [Publication] from +// [Session.Publish], a REQUEST_OK / REQUEST_ERROR before any Update, or a +// SUBSCRIBE_OK on a Subscription, is a second response to the request and +// closes the session (§5.1). +// - On a [NamespaceSubscription]'s broker, a NAMESPACE_DONE for a suffix no +// NAMESPACE announced closes the session (§10.19). +// - A second GOAWAY on the stream, or one with a New Session URI received +// by a server, closes the session (§10.4; see [RequestGoaways]). +// - Everything else (PUBLISH_DONE, other unsolicited responses, …) is +// handed to Serve's callback. // // Obtain one from a typed request handle's Broker method (e.g. // [Publication.Broker]) or [Session.NewRequestBroker]; from then on every @@ -47,13 +57,16 @@ type RequestBroker struct { // arrive in request order, so the waiter queue order must match the // write order. mu sync.Mutex - waiters []chan updateResult + waiters []updateWaiter // updatesClosed is latched when the stream's reader exits (or Close is // called); subsequent Update calls fail immediately instead of queueing // a waiter nothing will ever answer. Plain WriteMessage stays allowed — // e.g. a PUBLISH_DONE after the peer tore its side down. updatesClosed bool streamClosed bool + // updated records that Update sent a REQUEST_UPDATE, the only thing a + // REQUEST_OK or REQUEST_ERROR read here can answer. + updated bool // onUpdate decides each peer REQUEST_UPDATE; nil declines it. // onUpdateFailed runs after a declined update (§10.9.1). Both are set @@ -68,6 +81,22 @@ type RequestBroker struct { // See [RequestBroker.PeerMessages]. noPeerUpdate bool noPeerNotify bool + + // handle is the typed handle this broker came from, if any: Serve and + // Close report the subscription's end to it (see requestHandle.terminated). + handle *requestHandle + + // Serve's per-stream state, kept across calls: the stream's GOAWAYs + // (§10.4) when there is no handle to keep them, and on a + // SUBSCRIBE_NAMESPACE the current prefix and the full namespaces a + // NAMESPACE announced and no NAMESPACE_DONE withdrew (§10.19, §10.9.2). + goaways RequestGoaways + nsPrefix wire.TrackNamespace + namespaces map[track.Key]struct{} + // nsPrefixLost is set, under mu, once any Update gave up: responses no + // longer pair reliably, so the prefix is unknown and NAMESPACE_DONEs go + // unchecked. + nsPrefixLost bool } // PeerMessages declares whether the peer may send REQUEST_UPDATE (§10.9) and @@ -88,7 +117,8 @@ func (b *RequestBroker) UpdateScope(s message.ParamScope) { b.updateScope = s } // UpdateHandler decides a peer's REQUEST_UPDATE (§10.9). It returns the // REQUEST_OK to send, or an error: a *[RequestRejectedError] is sent as // REQUEST_ERROR with its code, reason and Retry Interval, any other error as -// INTERNAL_ERROR. +// INTERNAL_ERROR. So is a REDIRECT, which cannot answer a REQUEST_UPDATE +// (§10.6.2). type UpdateHandler func(upd *message.RequestUpdate) (*message.RequestOK, error) // HandleUpdates installs the handler that decides peer REQUEST_UPDATEs, @@ -132,7 +162,7 @@ func (b *RequestBroker) answerUpdate(upd *message.RequestUpdate) (bool, error) { return true, nil } rej, isRej := errors.AsType[*RequestRejectedError](err) - if !isRej { + if !isRej || rej.Code == moqt.RequestRedirect { rej = &RequestRejectedError{Code: moqt.RequestInternalError, Reason: err.Error()} } if werr := b.WriteMessage(&message.RequestError{ @@ -145,6 +175,65 @@ func (b *RequestBroker) answerUpdate(upd *message.RequestUpdate) (bool, error) { return false, nil } +// goawayState is the stream's GOAWAY checker: the handle's, shared with its +// other readers (e.g. [TrackSubscription.ReadPublishSkipped]), or the +// broker's own. +func (b *RequestBroker) goawayState() *RequestGoaways { + if b.handle != nil { + return &b.handle.goaways + } + return &b.goaways +} + +// namespaceFollowup tracks a NAMESPACE or NAMESPACE_DONE on a +// SUBSCRIBE_NAMESPACE's broker: "If a subscriber receives a NAMESPACE_DONE +// before the corresponding NAMESPACE, it MUST close the session with a +// 'PROTOCOL_VIOLATION'" (§10.19). Other brokers do not track them. +func (b *RequestBroker) namespaceFollowup(m message.Message) error { + if b.handle == nil || !b.handle.namespaces { + return nil + } + b.mu.Lock() + lost := b.nsPrefixLost + b.mu.Unlock() + if lost { + return nil + } + switch m := m.(type) { + case *message.Namespace: + if b.namespaces == nil { + b.namespaces = make(map[track.Key]struct{}) + } + b.namespaces[track.NewKey(slices.Concat(b.nsPrefix, m.TrackNamespaceSuffix), nil)] = struct{}{} + case *message.NamespaceDone: + key := track.NewKey(slices.Concat(b.nsPrefix, m.TrackNamespaceSuffix), nil) + if _, ok := b.namespaces[key]; !ok { + return b.sess.closeProtocolViolation(fmt.Errorf( + "moqt/session: NAMESPACE_DONE for %v before its NAMESPACE", m.TrackNamespaceSuffix)) + } + delete(b.namespaces, key) + } + return nil +} + +// answered is the request type (SUBSCRIBE or PUBLISH) of a broker on this +// side's request, whose response the peer has sent; zero otherwise. +func (b *RequestBroker) answered() message.Type { + if b.handle == nil { + return 0 + } + return b.handle.answered +} + +// updateWaiter is one sent REQUEST_UPDATE awaiting its §10.9 response and, +// when the update sets TRACK_NAMESPACE_PREFIX, the prefix it switches a +// namespace subscription to once accepted (§10.9.2). +type updateWaiter struct { + ch chan updateResult + prefix wire.TrackNamespace + setsPrefix bool +} + // updateResult carries one §10.9 response to a waiting Update call. type updateResult struct { ok *message.RequestOK @@ -178,7 +267,7 @@ func (s *Session) mapUpdateResponse(msg message.Message) (*message.RequestOK, er } return m, nil case *message.RequestError: - return nil, &RequestRejectedError{Code: m.ErrorCode, Reason: m.ErrorReason, RetryInterval: m.RetryInterval} + return nil, s.rejection(m, message.TypeRequestUpdate) default: return nil, fmt.Errorf("moqt/session: unexpected %s in REQUEST_UPDATE response", msg.Type()) } @@ -218,7 +307,13 @@ func (b *RequestBroker) Update(ctx context.Context, params message.Parameters) ( Parameters: params, }) if err == nil { - b.waiters = append(b.waiters, ch) + w := updateWaiter{ch: ch} + if p, ok := params.Find(message.ParamTrackNamespacePrefix); ok { + prefix, perr := message.TrackNamespacePrefixFromParam(p) + w.prefix, w.setsPrefix = prefix, perr == nil + } + b.waiters = append(b.waiters, w) + b.updated = true } b.mu.Unlock() if err != nil { @@ -230,7 +325,13 @@ func (b *RequestBroker) Update(ctx context.Context, params message.Parameters) ( return res.ok, res.err case <-ctx.Done(): b.mu.Lock() - if i := slices.Index(b.waiters, ch); i >= 0 { + if i := slices.IndexFunc(b.waiters, func(w updateWaiter) bool { return w.ch == ch }); i >= 0 { + // Its late answer will pair with the next update, so on a + // SUBSCRIBE_NAMESPACE where a prefix update's REQUEST_OK falls + // among the NAMESPACEs (§10.9.2) is no longer known. + if b.handle != nil && b.handle.namespaces { + b.nsPrefixLost = true + } b.waiters = slices.Delete(b.waiters, i, i+1) } b.mu.Unlock() @@ -289,7 +390,7 @@ func (b *RequestBroker) route(msg message.Message) bool { b.mu.Unlock() return false } - var recipients []chan updateResult + var recipients []updateWaiter if _, isErr := msg.(*message.RequestError); isErr { recipients, b.waiters = b.waiters, nil } else { @@ -298,9 +399,14 @@ func (b *RequestBroker) route(msg message.Message) bool { b.mu.Unlock() ok, err := b.sess.mapUpdateResponse(msg) + if err == nil && recipients[0].setsPrefix { + // §10.9.2: suffixes after this REQUEST_OK are relative to the new + // prefix. Only Serve's goroutine reads nsPrefix. + b.nsPrefix = recipients[0].prefix + } res := updateResult{ok: ok, err: err} - for _, ch := range recipients { - ch <- res + for _, w := range recipients { + w.ch <- res } return true } @@ -314,16 +420,21 @@ func (b *RequestBroker) closeUpdates() { b.waiters = nil b.updatesClosed = true b.mu.Unlock() - for _, ch := range waiters { - ch <- updateResult{err: ErrRequestStreamClosed} + for _, w := range waiters { + w.ch <- updateResult{err: ErrRequestStreamClosed} } } // Close cancels the request (§3.3.3): pending and future Updates fail with // [ErrRequestStreamClosed] and both directions are reset with code, which -// unblocks a running Serve. Serialized against in-flight writes; idempotent. +// unblocks a running Serve. On a broker from a [Subscription] or +// [IncomingPublication] the subscription is Terminated (§5.1) and its Track +// Alias released (§11.1). Serialized against in-flight writes; idempotent. // Must not be called with locks that Serve's callback might need held. func (b *RequestBroker) Close(code moqt.StreamResetCode) { + if b.handle != nil { + b.handle.terminated() + } b.closeUpdates() b.mu.Lock() defer b.mu.Unlock() @@ -335,6 +446,66 @@ func (b *RequestBroker) Close(code moqt.StreamResetCode) { b.stream.CancelWrite(uint64(code)) } +// receiveUpdate checks and answers a peer REQUEST_UPDATE for Serve (§10.9). +// A non-nil error means Serve stops; a violation closed the session. +func (b *RequestBroker) receiveUpdate(m *message.RequestUpdate, updates *RequestUpdateLimiter) error { + if b.noPeerUpdate { + return b.sess.closeProtocolViolation(errors.New( + "moqt/session: REQUEST_UPDATE from a peer that may not send one")) + } + if b.updateScope != 0 { + if err := b.sess.CheckPeerParams(b.updateScope, m); err != nil { + return err + } + } + // §10.1: a REQUEST_UPDATE consumes a Request ID from the sender's space; + // a wrong-parity or duplicate ID is session-fatal. + if err := b.sess.CheckPeerRequestID(m.RequestID); err != nil { + _ = b.sess.Close(moqt.SessionInvalidRequestID, err.Error()) + return err + } + // §10.3.1.7: reject a REQUEST_UPDATE that exceeds the per-stream + // MAX_REQUEST_UPDATES limit before acting on it. + if err := updates.Received(); err != nil { + _ = b.sess.Close(moqt.SessionTooManyRequestUpdates, err.Error()) + return err + } + // §10.9: "MUST respond with exactly one REQUEST_OK or REQUEST_ERROR". + // onMsg still observes the update. + accepted, err := b.answerUpdate(m) + if err != nil { + return err + } + updates.Responded() + if !accepted && b.onUpdateFailed != nil { + b.onUpdateFailed() + } + return nil +} + +// readFailed ends Serve on a read error: nil on the peer's FIN, ctx.Err() on +// cancellation, and err otherwise, after resetting the read side. +func (b *RequestBroker) readFailed(ctx context.Context, err error) error { + switch { + case ctx.Err() != nil: + return ctx.Err() + case errors.Is(err, io.EOF): + if b.handle != nil { + b.handle.peerFinished() + } + return nil + case errors.Is(err, message.ErrMalformedMessage): + // §10, and §10.2 for an unknown parameter. + b.stream.CancelRead(uint64(moqt.StreamResetInternalError)) + return b.sess.closeProtocolViolation(err) + default: + // Covers peer resets too (a STOP_SENDING on an already-reset stream + // is a transport no-op). + b.stream.CancelRead(uint64(moqt.StreamResetInternalError)) + return err + } +} + // Serve owns every read on the request stream until the peer tears it down // (EOF / reset), ctx is cancelled (the read side is then reset to unblock // the parse), or onMsg returns false. On exit, pending and future Update @@ -342,16 +513,27 @@ func (b *RequestBroker) Close(code moqt.StreamResetCode) { // // Responses route to Update waiters; a token cache fault closes the session // (§10.2.2); peer REQUEST_UPDATEs are answered as described on -// [RequestBroker]. Every other message, including each REQUEST_UPDATE and any -// unsolicited response, is passed to onMsg (nil means "discard"); return false -// from onMsg to stop serving. +// [RequestBroker], and a second response to this side's SUBSCRIBE or PUBLISH +// closes the session (§5.1). Every other message, including each +// REQUEST_UPDATE and any other unsolicited response, is passed to onMsg (nil +// means "discard"); return false from onMsg to stop serving. // // A read error resets the read side with INTERNAL_ERROR; a malformed follow-up // also closes the session with PROTOCOL_VIOLATION (§10). Serve returns nil on // a clean FIN or an onMsg stop, ctx.Err() on cancellation, and the read/token // error otherwise. +// +// On a broker from a [Subscription] or [IncomingPublication], every exit but +// an onMsg stop Terminates the subscription (§5.1) and releases its Track +// Alias (§11.1). func (b *RequestBroker) Serve(ctx context.Context, onMsg func(message.Message) bool) error { defer b.closeUpdates() + stopped := false // by onMsg: the stream may still carry the subscription + defer func() { + if !stopped && b.handle != nil { + b.handle.terminated() + } + }() stop := context.AfterFunc(ctx, func() { b.stream.CancelRead(uint64(moqt.StreamResetSessionClosed)) }) @@ -364,21 +546,7 @@ func (b *RequestBroker) Serve(ctx context.Context, onMsg func(message.Message) b for { msg, err := message.Parse(b.stream) if err != nil { - switch { - case ctx.Err() != nil: - return ctx.Err() - case errors.Is(err, io.EOF): - return nil - case errors.Is(err, message.ErrMalformedMessage): - // §10, and §10.2 for an unknown parameter. - b.stream.CancelRead(uint64(moqt.StreamResetInternalError)) - return b.sess.closeProtocolViolation(err) - default: - // Covers peer resets too (a STOP_SENDING on an - // already-reset stream is a transport no-op). - b.stream.CancelRead(uint64(moqt.StreamResetInternalError)) - return err - } + return b.readFailed(ctx, err) } // §10.2.2: follow-ups may REGISTER/DELETE token aliases; skipping @@ -392,13 +560,42 @@ func (b *RequestBroker) Serve(ctx context.Context, onMsg func(message.Message) b } switch m := msg.(type) { + case *message.SubscribeOK: + // §5.1: the response to this side's SUBSCRIBE was read already, + // and "The peer SHOULD close the session with a protocol error if + // it receives more than one." + if b.answered() == message.TypeSubscribe { + return b.sess.closeProtocolViolation(errors.New("moqt/session: SUBSCRIBE_OK after the response")) + } case *message.RequestOK, *message.RequestError: // The request's own response was read before the broker // attached, so every REQUEST_OK here is a REQUEST_UPDATE_OK - // (§10.5), even one whose Update gave up. + // (§10.5), even one whose Update gave up. Before any Update, on + // this side's SUBSCRIBE or PUBLISH, it is a second response to + // the request, a second PUBLISH_OK among them (§5.1). + if b.answered() != 0 { + b.mu.Lock() + updated := b.updated + b.mu.Unlock() + if !updated { + return b.sess.closeProtocolViolation( + fmt.Errorf("moqt/session: %s after the response, before any REQUEST_UPDATE", m.Type())) + } + } if err := b.sess.checkRequestOKTrackProperties(nil, m); err != nil { return err } + // §10.6.1, before routing: an unsolicited REQUEST_ERROR reaches + // onMsg instead of an Update. + if e, isErr := m.(*message.RequestError); isErr { + if err := redirectViolation( + e.Redirect, + message.TypeRequestUpdate, + b.sess.role == roleServer, + ); err != nil { + return b.sess.closeProtocolViolation(fmt.Errorf("moqt/session: received %w", err)) + } + } if err := b.sess.CheckPeerParams(message.ScopeRequestUpdateOK, m); err != nil { return err } @@ -406,6 +603,14 @@ func (b *RequestBroker) Serve(ctx context.Context, onMsg func(message.Message) b continue } // Unsolicited response — surface via onMsg below. + case *message.Goaway: + if err := b.goawayState().Received(b.sess, m); err != nil { + return err + } + case *message.Namespace, *message.NamespaceDone: + if err := b.namespaceFollowup(m); err != nil { + return err + } case *message.PublishStateNotify: if b.noPeerNotify { return b.sess.closeProtocolViolation(errors.New( @@ -415,41 +620,13 @@ func (b *RequestBroker) Serve(ctx context.Context, onMsg func(message.Message) b return err } case *message.RequestUpdate: - if b.noPeerUpdate { - return b.sess.closeProtocolViolation(errors.New( - "moqt/session: REQUEST_UPDATE from a peer that may not send one")) - } - if b.updateScope != 0 { - if err := b.sess.CheckPeerParams(b.updateScope, m); err != nil { - return err - } - } - // §10.1: a REQUEST_UPDATE consumes a Request ID from the - // sender's space; a wrong-parity or duplicate ID is - // session-fatal. - if err := b.sess.CheckPeerRequestID(m.RequestID); err != nil { - _ = b.sess.Close(moqt.SessionInvalidRequestID, err.Error()) - return err - } - // §10.3.1.7: reject a REQUEST_UPDATE that exceeds the per-stream - // MAX_REQUEST_UPDATES limit before acting on it. - if err := updates.Received(); err != nil { - _ = b.sess.Close(moqt.SessionTooManyRequestUpdates, err.Error()) - return err - } - // §10.9: "MUST respond with exactly one REQUEST_OK or - // REQUEST_ERROR". onMsg still observes the update. - accepted, err := b.answerUpdate(m) - if err != nil { + if err := b.receiveUpdate(m, updates); err != nil { return err } - updates.Responded() - if !accepted && b.onUpdateFailed != nil { - b.onUpdateFailed() - } } if onMsg != nil && !onMsg(msg) { + stopped = true return nil } } diff --git a/pkg/moqt/session/example_test.go b/pkg/moqt/session/example_test.go index 800ff8e3..75f172e9 100644 --- a/pkg/moqt/session/example_test.go +++ b/pkg/moqt/session/example_test.go @@ -436,18 +436,17 @@ func ExampleSession_SubscribeNamespace() { } defer annStream.Close() - for { - msg, err := message.Parse(annStream) - if err != nil { - return - } + // The broker enforces the session-level rules, such as a NAMESPACE_DONE + // only after its NAMESPACE (§10.19), and returns when the stream ends. + _ = annStream.Broker().Serve(ctx, func(msg message.Message) bool { switch m := msg.(type) { case *message.Namespace: fmt.Printf("announced: %v\n", m.TrackNamespaceSuffix) case *message.NamespaceDone: fmt.Printf("done: %v\n", m.TrackNamespaceSuffix) } - } + return true + }) } // Accepting requests on the server side. A server (or relay) pulls inbound diff --git a/pkg/moqt/session/first_response_test.go b/pkg/moqt/session/first_response_test.go new file mode 100644 index 00000000..4ef41ab1 --- /dev/null +++ b/pkg/moqt/session/first_response_test.go @@ -0,0 +1,80 @@ +package session_test + +import ( + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt" + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/session" + "github.com/floatdrop/moq-go/pkg/moqt/wire" +) + +// TestNamespaceScopedFirstResponseCloses: a first response other than +// REQUEST_OK or REQUEST_ERROR to SUBSCRIBE_NAMESPACE or SUBSCRIBE_TRACKS closes +// the session with PROTOCOL_VIOLATION (§10.19, §10.20), even one that is legal +// later on the stream. +func TestNamespaceScopedFirstResponseCloses(t *testing.T) { + prefix := wire.TrackNamespace{[]byte("ns")} + requests := []struct { + name string + open func(t *testing.T, s *session.Session) error + }{ + {"SUBSCRIBE_NAMESPACE", func(t *testing.T, s *session.Session) error { + _, err := s.SubscribeNamespace(t.Context(), &message.SubscribeNamespace{TrackNamespacePrefix: prefix}) + return err + }}, + {"SUBSCRIBE_TRACKS", func(t *testing.T, s *session.Session) error { + _, err := s.SubscribeTracks(t.Context(), &message.SubscribeTracks{TrackNamespacePrefix: prefix}) + return err + }}, + } + responses := []struct { + name string + msg message.Message + }{ + {"NAMESPACE", &message.Namespace{TrackNamespaceSuffix: wire.TrackNamespace{[]byte("a")}}}, + {"GOAWAY", &message.Goaway{}}, + {"SUBSCRIBE_OK", &message.SubscribeOK{TrackAlias: 1}}, + } + for _, req := range requests { + for _, resp := range responses { + t.Run(req.name+" answered with "+resp.name, func(t *testing.T) { + cli, srv := openPair(t) + answerRaw(t, srv, resp.msg) + if err := req.open(t, cli); err == nil { + t.Fatalf("%s succeeded on a %s", req.name, resp.name) + } + requireClosedCode(t, cli, moqt.SessionProtocolViolation) + }) + } + } +} + +// TestOtherFirstResponseKeepsSession: no MUST covers the first response to +// other requests (§5.1, §6.2 put theirs on the sender), so an unexpected one +// fails the request and leaves the session open. +func TestOtherFirstResponseKeepsSession(t *testing.T) { + for _, tc := range []struct { + name string + open func(t *testing.T, s *session.Session) error + }{ + {"SUBSCRIBE", func(t *testing.T, s *session.Session) error { + _, err := s.Subscribe(t.Context(), &message.Subscribe{Name: []byte("t")}) + return err + }}, + {"PUBLISH_NAMESPACE", func(t *testing.T, s *session.Session) error { + _, err := s.PublishNamespace(t.Context(), &message.PublishNamespace{Namespace: wire.TrackNamespace{[]byte("ns")}}) + return err + }}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + answerRaw(t, srv, &message.Namespace{TrackNamespaceSuffix: wire.TrackNamespace{[]byte("a")}}) + if err := tc.open(t, cli); err == nil { + t.Fatalf("%s succeeded on a NAMESPACE", tc.name) + } + requireStaysOpen(t, cli, 50*time.Millisecond) + }) + } +} diff --git a/pkg/moqt/session/goaway.go b/pkg/moqt/session/goaway.go index d0db817d..6f569ad8 100644 --- a/pkg/moqt/session/goaway.go +++ b/pkg/moqt/session/goaway.go @@ -2,6 +2,7 @@ package session import ( "errors" + "fmt" "time" "github.com/floatdrop/moq-go/pkg/moqt/message" @@ -86,12 +87,9 @@ func (s *Session) handleGoaway(m *message.Goaway) error { s.mu.Unlock() return errors.New("duplicate GOAWAY on control stream") } - // §10.4: a client cannot direct a server to migrate, so a non-empty URI - // from a client is a PROTOCOL_VIOLATION. From our perspective, that means - // if we are the server we must reject a GOAWAY with a URI. - if s.role == roleServer && len(m.NewSessionURI) > 0 { + if err := s.checkGoawayURI(m); err != nil { s.mu.Unlock() - return errors.New("GOAWAY from client carries non-empty URI") + return err } s.goawayReceived = m // Snapshot the registered handler under the lock and mark it fired so a @@ -110,3 +108,39 @@ func (s *Session) handleGoaway(m *message.Goaway) error { } return nil } + +// checkGoawayURI enforces §10.4 "If a server receives a GOAWAY with a +// non-zero New Session URI Length it MUST close the session with a +// PROTOCOL_VIOLATION": a client cannot direct a server to migrate. +func (s *Session) checkGoawayURI(m *message.Goaway) error { + if s.role == roleServer && len(m.NewSessionURI) > 0 { + return errors.New("GOAWAY from client carries non-empty URI") + } + return nil +} + +// RequestGoaways checks the GOAWAYs one request stream carries (§10.4): "The +// endpoint MUST close the session with a PROTOCOL_VIOLATION ... if it receives +// more than one GOAWAY on the control stream or on a single request stream", +// and a server closes on one carrying a New Session URI. The zero value is +// ready for a stream; callers that read a request stream with [message.Parse] +// keep one per stream. +// +// Not safe for concurrent use, which matches the single reader of a request +// stream ([RequestBroker.Serve] and the relay's per-stream readers). +type RequestGoaways struct { + seen bool +} + +// Received checks a GOAWAY read off the stream. On a violation it closes s +// with PROTOCOL_VIOLATION and returns the error. +func (g *RequestGoaways) Received(s *Session, m *message.Goaway) error { + if g.seen { + return s.closeProtocolViolation(errors.New("moqt/session: second GOAWAY on a request stream")) + } + g.seen = true + if err := s.checkGoawayURI(m); err != nil { + return s.closeProtocolViolation(fmt.Errorf("moqt/session: request stream %w", err)) + } + return nil +} diff --git a/pkg/moqt/session/namespace.go b/pkg/moqt/session/namespace.go index 68b37dbe..702c2b72 100644 --- a/pkg/moqt/session/namespace.go +++ b/pkg/moqt/session/namespace.go @@ -30,45 +30,54 @@ func (p *NamespacePublication) Close() error { // NamespaceSubscription is an established SUBSCRIBE_NAMESPACE request (§10.19). // It embeds the still-open request stream and carries the peer's REQUEST_OK; -// NAMESPACE / NAMESPACE_DONE notifications arrive on the embedded stream. -// Read it with [RequestBroker.Serve], which enforces the session-level rules -// (§10, §10.2.1); a caller using message.Parse must apply them itself. +// NAMESPACE / NAMESPACE_DONE notifications arrive on the embedded stream. Read +// it with its [NamespaceSubscription.Broker]'s [RequestBroker.Serve], which +// enforces the session-level rules: §10 and §10.2.1, a NAMESPACE_DONE only +// after its NAMESPACE (§10.19), and no PUBLISH_STATE_NOTIFY or REQUEST_UPDATE +// from the publisher (§10.9, §10.10). The NAMESPACE_DONE check follows a +// TRACK_NAMESPACE_PREFIX update from the REQUEST_OK that accepts it (§10.9.2). +// A caller using message.Parse must apply these rules itself. +// [NamespaceSubscription.Update] modifies the subscription (§10.9); Close ends +// it (§6.1, §3.3.3). type NamespaceSubscription struct { - // Stream is the SUBSCRIBE_NAMESPACE request stream, still open to receive - // NAMESPACE / NAMESPACE_DONE notifications. [NamespaceSubscription.Close] - // ends the subscription. - Stream + // requestHandle carries the SUBSCRIBE_NAMESPACE request stream, still + // open to receive NAMESPACE / NAMESPACE_DONE notifications. + requestHandle // OK is the REQUEST_OK the peer replied with. OK *message.RequestOK } -// Close ends the subscription by cancelling the request (§6.1, §3.3.3). -func (n *NamespaceSubscription) Close() error { - cancelRequest(n.Stream) - return nil -} - // TrackSubscription is an established SUBSCRIBE_TRACKS request (§10.20). It // embeds the still-open request stream and carries the peer's REQUEST_OK. // Follow-up PUBLISH_SKIPPED notifications are read via -// [TrackSubscription.ReadPublishSkipped]. +// [TrackSubscription.ReadPublishSkipped], or its [TrackSubscription.Broker]'s +// [RequestBroker.Serve], never both at once. Both close the session on a +// PUBLISH_STATE_NOTIFY or REQUEST_UPDATE from the publisher (§10.9, §10.10). +// [TrackSubscription.Update] modifies the subscription (§10.9); Close ends it +// (§6.1, §3.3.3). type TrackSubscription struct { - // Stream is the SUBSCRIBE_TRACKS request stream, still open to receive - // PUBLISH_SKIPPED follow-ups. [TrackSubscription.Close] ends the - // subscription. - Stream + // requestHandle carries the SUBSCRIBE_TRACKS request stream, still open + // to receive PUBLISH_SKIPPED follow-ups. + requestHandle // OK is the REQUEST_OK the peer replied with. OK *message.RequestOK +} - s *Session +// Update sends a REQUEST_UPDATE (§10.9), e.g. a new TRACK_NAMESPACE_PREFIX +// (§10.9.2), through the subscription's [NamespaceSubscription.Broker], whose +// Serve must be running to deliver the answer. Reading the answer directly +// would take a NAMESPACE for it. +func (n *NamespaceSubscription) Update(ctx context.Context, params message.Parameters) (*message.RequestOK, error) { + return n.Broker().Update(ctx, params) } -// Close ends the subscription by cancelling the request (§6.1, §3.3.3). -func (t *TrackSubscription) Close() error { - cancelRequest(t.Stream) - return nil +// Update sends a REQUEST_UPDATE (§10.9) through the subscription's +// [TrackSubscription.Broker], whose Serve must be running to deliver the +// answer, as for [NamespaceSubscription.Update]. +func (t *TrackSubscription) Update(ctx context.Context, params message.Parameters) (*message.RequestOK, error) { + return t.Broker().Update(ctx, params) } // PublishNamespace opens a PUBLISH_NAMESPACE request stream (§10.16) and @@ -94,14 +103,18 @@ func (s *Session) PublishNamespace( // // On success a [NamespaceSubscription] is returned whose embedded stream stays // open (the caller will receive NAMESPACE / NAMESPACE_DONE messages on it). On -// REQUEST_ERROR the stream is closed and a *RequestRejectedError is returned. +// REQUEST_ERROR the stream is closed and a *RequestRejectedError is returned; +// any other first response closes the session with PROTOCOL_VIOLATION (§10.19). func (s *Session) SubscribeNamespace( ctx context.Context, m *message.SubscribeNamespace, ) (*NamespaceSubscription, error) { return awaitRequestResponse(ctx, s, m, func(stream Stream, ok *message.RequestOK) (*NamespaceSubscription, error) { - return &NamespaceSubscription{Stream: stream, OK: ok}, nil + return &NamespaceSubscription{ + Stream: stream, s: s, requestID: m.RequestID, + namespaces: true, nsPrefix: m.TrackNamespacePrefix, OK: ok, + }, nil }) } @@ -111,11 +124,16 @@ func (s *Session) SubscribeNamespace( // // On success a [TrackSubscription] is returned whose embedded stream stays open // for PUBLISH_SKIPPED follow-ups (read via [TrackSubscription.ReadPublishSkipped]). -// On REQUEST_ERROR the stream is closed and a *RequestRejectedError is returned. +// On REQUEST_ERROR the stream is closed and a *RequestRejectedError is returned; +// any other first response closes the session with PROTOCOL_VIOLATION (§10.20). func (s *Session) SubscribeTracks(ctx context.Context, m *message.SubscribeTracks) (*TrackSubscription, error) { return awaitRequestResponse(ctx, s, m, func(stream Stream, ok *message.RequestOK) (*TrackSubscription, error) { - return &TrackSubscription{Stream: stream, OK: ok, s: s}, nil + if err := s.checkTrackPropertyValues(ok.TrackProperties, "SUBSCRIBE_TRACKS_OK"); err != nil { + cancelRequest(stream) + return nil, err + } + return &TrackSubscription{Stream: stream, s: s, requestID: m.RequestID, OK: ok}, nil }) } @@ -220,20 +238,43 @@ func (r *Request) AcceptSubscribeTracks() (*IncomingTrackSubscription, error) { // message names the track the publisher couldn't push; the caller's sanctioned // recovery is to issue an explicit SUBSCRIBE for it. // -// It blocks until a message arrives or the stream ends. A non-PUBLISH_SKIPPED -// message is reported as an error, as is the underlying read error (e.g. +// It cannot be combined with [TrackSubscription.Update], whose answer only the +// broker's Serve delivers. +// +// It blocks until a PUBLISH_SKIPPED arrives or the stream ends. A single +// GOAWAY is skipped (§10.4); a caller that would re-issue the request at its +// New Session URI reads with the broker instead, whose Serve hands it over. A +// second GOAWAY, and a PUBLISH_STATE_NOTIFY or REQUEST_UPDATE from the +// publisher (§10.9, §10.10), close the session with PROTOCOL_VIOLATION. Any +// other message is reported as an error, as is the underlying read error (e.g. // io.EOF when the publisher FINs the SUBSCRIBE_TRACKS stream). func (t *TrackSubscription) ReadPublishSkipped() (*message.PublishSkipped, error) { - m, err := message.Parse(t.Stream) - if errors.Is(err, message.ErrMalformedMessage) { - return nil, t.s.closeProtocolViolation(fmt.Errorf("moqt/session: read SUBSCRIBE_TRACKS follow-up: %w", err)) - } - if err != nil { - return nil, fmt.Errorf("moqt/session: read SUBSCRIBE_TRACKS follow-up: %w", err) - } - pb, ok := m.(*message.PublishSkipped) - if !ok { - return nil, fmt.Errorf("moqt/session: unexpected %s on SUBSCRIBE_TRACKS stream, want PUBLISH_SKIPPED", m.Type()) + for { + m, err := message.Parse(t) + if errors.Is(err, message.ErrMalformedMessage) { + return nil, t.s.closeProtocolViolation(fmt.Errorf("moqt/session: read SUBSCRIBE_TRACKS follow-up: %w", err)) + } + if err != nil { + return nil, fmt.Errorf("moqt/session: read SUBSCRIBE_TRACKS follow-up: %w", err) + } + switch m := m.(type) { + case *message.PublishSkipped: + return m, nil + case *message.Goaway: + // §10.4: legal once; the request is not migrated. + if err := t.goaways.Received(t.s, m); err != nil { + return nil, err + } + case *message.PublishStateNotify, *message.RequestUpdate: + // §10.10: PUBLISH_STATE_NOTIFY "applies only to subscriptions"; + // §10.9: the publisher did not send this request. + return nil, t.s.closeProtocolViolation( + fmt.Errorf("moqt/session: %s on a SUBSCRIBE_TRACKS stream", m.Type())) + default: + return nil, fmt.Errorf( + "moqt/session: unexpected %s on SUBSCRIBE_TRACKS stream, want PUBLISH_SKIPPED", + m.Type(), + ) + } } - return pb, nil } diff --git a/pkg/moqt/session/namespace_followup_test.go b/pkg/moqt/session/namespace_followup_test.go new file mode 100644 index 00000000..c9b9d449 --- /dev/null +++ b/pkg/moqt/session/namespace_followup_test.go @@ -0,0 +1,339 @@ +package session_test + +import ( + "context" + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt" + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/session" + "github.com/floatdrop/moq-go/pkg/moqt/wire" +) + +// namespaceSubscribePair has cli SUBSCRIBE_NAMESPACE srv, and returns the +// subscription and the stream srv answers on. +func namespaceSubscribePair(t *testing.T, cli, srv *session.Session) (*session.NamespaceSubscription, session.Stream) { + t.Helper() + streams := make(chan session.Stream, 1) + go func() { + defer close(streams) + r, err := srv.AcceptRequest(t.Context()) + if err != nil { + return + } + if in, err := r.AcceptSubscribeNamespace(); err == nil { + streams <- in.Stream + } + }() + sub, err := cli.SubscribeNamespace(t.Context(), &message.SubscribeNamespace{ + TrackNamespacePrefix: wire.TrackNamespace{[]byte("ns")}, + }) + must(t, err) + stream, ok := <-streams + if !ok { + t.Fatal("server failed to accept the SUBSCRIBE_NAMESPACE") + } + return sub, stream +} + +// trackSubscribePair is namespaceSubscribePair for SUBSCRIBE_TRACKS. +func trackSubscribePair(t *testing.T, cli, srv *session.Session) (*session.TrackSubscription, session.Stream) { + t.Helper() + streams := make(chan session.Stream, 1) + go func() { + defer close(streams) + r, err := srv.AcceptRequest(t.Context()) + if err != nil { + return + } + if in, err := r.AcceptSubscribeTracks(); err == nil { + streams <- in.Stream + } + }() + sub, err := cli.SubscribeTracks(t.Context(), &message.SubscribeTracks{ + TrackNamespacePrefix: wire.TrackNamespace{[]byte("ns")}, + }) + must(t, err) + stream, ok := <-streams + if !ok { + t.Fatal("server failed to accept the SUBSCRIBE_TRACKS") + } + return sub, stream +} + +// send writes msgs to stream in the background, in order. +func send(stream session.Stream, msgs ...message.Message) { + go func() { + for _, m := range msgs { + if err := message.Marshal(stream, m); err != nil { + return + } + } + }() +} + +func nsAnnounce(suffix string) *message.Namespace { + return &message.Namespace{TrackNamespaceSuffix: wire.TrackNamespace{[]byte(suffix)}} +} + +func nsDone(suffix string) *message.NamespaceDone { + return &message.NamespaceDone{TrackNamespaceSuffix: wire.TrackNamespace{[]byte(suffix)}} +} + +// serveBriefly runs b.Serve until it returns or 2s pass. +func serveBriefly(t *testing.T, b *session.RequestBroker, onMsg func(message.Message) bool) { + t.Helper() + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel() + _ = b.Serve(ctx, onMsg) +} + +// TestNamespaceSubscriptionViolationCloses: on a NamespaceSubscription's +// broker a NAMESPACE_DONE with no NAMESPACE before it (§10.19), and a +// PUBLISH_STATE_NOTIFY (§10.10) or REQUEST_UPDATE (§10.9) from the publisher, +// close the session with PROTOCOL_VIOLATION. +func TestNamespaceSubscriptionViolationCloses(t *testing.T) { + for _, tc := range []struct { + name string + msgs []message.Message + }{ + {"NAMESPACE_DONE first", []message.Message{nsDone("a")}}, + {"NAMESPACE_DONE for another suffix", []message.Message{nsAnnounce("a"), nsDone("b")}}, + {"NAMESPACE_DONE twice", []message.Message{nsAnnounce("a"), nsDone("a"), nsDone("a")}}, + {"PUBLISH_STATE_NOTIFY", []message.Message{&message.PublishStateNotify{}}}, + {"REQUEST_UPDATE", []message.Message{&message.RequestUpdate{RequestID: 1}}}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + sub, stream := namespaceSubscribePair(t, cli, srv) + send(stream, tc.msgs...) + serveBriefly(t, sub.Broker(), nil) + requireClosedCode(t, cli, moqt.SessionProtocolViolation) + }) + } +} + +// TestNamespaceSubscriptionFollowupsDelivered: NAMESPACE and NAMESPACE_DONE in +// order reach Serve's callback, including a NAMESPACE_DONE read by a later +// Serve call than its NAMESPACE, and a re-announced suffix. +func TestNamespaceSubscriptionFollowupsDelivered(t *testing.T) { + cli, srv := openPair(t) + sub, stream := namespaceSubscribePair(t, cli, srv) + b := sub.Broker() + send(stream, nsAnnounce("a"), nsDone("a"), nsAnnounce("a"), nsDone("a")) + + var got []message.Type + collect := func(n int) func(message.Message) bool { + return func(m message.Message) bool { + got = append(got, m.Type()) + return len(got) < n + } + } + serveBriefly(t, b, collect(1)) // stops after the first NAMESPACE + serveBriefly(t, b, collect(4)) + want := []message.Type{ + message.TypeNamespace, + message.TypeNamespaceDone, + message.TypeNamespace, + message.TypeNamespaceDone, + } + if len(got) != len(want) { + t.Fatalf("callback saw %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("callback saw %v, want %v", got, want) + } + } + requireStaysOpen(t, cli, 50*time.Millisecond) +} + +// TestTrackSubscriptionPeerMessagesClose: a PUBLISH_STATE_NOTIFY (§10.10) or +// REQUEST_UPDATE (§10.9) from the publisher of a SUBSCRIBE_TRACKS closes the +// session with PROTOCOL_VIOLATION, read by ReadPublishSkipped or the broker. +func TestTrackSubscriptionPeerMessagesClose(t *testing.T) { + for _, served := range []bool{false, true} { + for _, msg := range []message.Message{&message.PublishStateNotify{}, &message.RequestUpdate{RequestID: 1}} { + name := msg.Type().String() + " read by ReadPublishSkipped" + if served { + name = msg.Type().String() + " served" + } + t.Run(name, func(t *testing.T) { + cli, srv := openPair(t) + sub, stream := trackSubscribePair(t, cli, srv) + send(stream, msg) + if served { + serveBriefly(t, sub.Broker(), nil) + } else if _, err := sub.ReadPublishSkipped(); err == nil { + t.Fatalf("ReadPublishSkipped returned a PUBLISH_SKIPPED for a %s", msg.Type()) + } + requireClosedCode(t, cli, moqt.SessionProtocolViolation) + }) + } + } +} + +// TestReadPublishSkippedGoaway: ReadPublishSkipped skips a single GOAWAY +// (§10.4) to return the PUBLISH_SKIPPED after it, and closes the session on a +// second. +func TestReadPublishSkippedGoaway(t *testing.T) { + skipped := &message.PublishSkipped{TrackNamespaceSuffix: wire.TrackNamespace{[]byte("a")}, TrackName: []byte("t")} + t.Run("one", func(t *testing.T) { + cli, srv := openPair(t) + sub, stream := trackSubscribePair(t, cli, srv) + send(stream, &message.Goaway{}, skipped) + got, err := sub.ReadPublishSkipped() + if err != nil { + t.Fatalf("ReadPublishSkipped: %v", err) + } + if string(got.TrackName) != "t" { + t.Fatalf("PUBLISH_SKIPPED for %q, want \"t\"", got.TrackName) + } + requireStaysOpen(t, cli, 50*time.Millisecond) + }) + t.Run("two", func(t *testing.T) { + cli, srv := openPair(t) + sub, stream := trackSubscribePair(t, cli, srv) + send(stream, &message.Goaway{}, &message.Goaway{}, skipped) + if _, err := sub.ReadPublishSkipped(); err == nil { + t.Fatal("ReadPublishSkipped returned past a second GOAWAY") + } + requireClosedCode(t, cli, moqt.SessionProtocolViolation) + }) +} + +// TestNamespaceSubscriptionUpdate: the subscriber may update a +// SUBSCRIBE_NAMESPACE (§10.9), and its broker pairs the answer. +func TestNamespaceSubscriptionUpdate(t *testing.T) { + cli, srv := openPair(t) + sub, stream := namespaceSubscribePair(t, cli, srv) + go func() { + if _, err := message.Parse(stream); err != nil { + return + } + _ = message.Marshal(stream, &message.RequestOK{}) + }() + b := sub.Broker() + go func() { _ = b.Serve(t.Context(), nil) }() + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel() + if _, err := sub.Update(ctx, nil); err != nil { + t.Fatalf("Update: %v", err) + } +} + +// TestNamespaceSubscriptionPrefixUpdate: NAMESPACE_DONE suffixes after the +// REQUEST_OK accepting a TRACK_NAMESPACE_PREFIX update are relative to the new +// prefix (§10.9.2), so the §10.19 check resolves them there; before it, or +// after a REQUEST_ERROR, against the old one. +func TestNamespaceSubscriptionPrefixUpdate(t *testing.T) { + ab := &message.Namespace{TrackNamespaceSuffix: wire.TrackNamespace{[]byte("a"), []byte("b")}} + for _, tc := range []struct { + name string + answer message.Message + done *message.NamespaceDone + closes bool + }{ + {"accepted, new-prefix suffix", &message.RequestOK{}, nsDone("b"), false}, + {"accepted, old-prefix suffix", &message.RequestOK{}, + &message.NamespaceDone{TrackNamespaceSuffix: wire.TrackNamespace{[]byte("a"), []byte("b")}}, true}, + {"rejected, old-prefix suffix", &message.RequestError{ErrorCode: moqt.RequestInternalError}, + &message.NamespaceDone{TrackNamespaceSuffix: wire.TrackNamespace{[]byte("a"), []byte("b")}}, false}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + sub, stream := namespaceSubscribePair(t, cli, srv) // prefix {ns} + go func() { + if message.Marshal(stream, ab) != nil { + return + } + if _, err := message.Parse(stream); err != nil { // the REQUEST_UPDATE + return + } + _ = message.Marshal(stream, tc.answer) + _ = message.Marshal(stream, tc.done) + }() + b := sub.Broker() + go func() { _ = b.Serve(t.Context(), nil) }() + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel() + _, _ = sub.Update(ctx, message.Parameters{ + message.TrackNamespacePrefixParam(wire.TrackNamespace{[]byte("ns"), []byte("a")}), + }) + if tc.closes { + requireClosedCode(t, cli, moqt.SessionProtocolViolation) + } else { + requireStaysOpen(t, cli, 100*time.Millisecond) + } + }) + } +} + +// TestNamespaceSubscriptionLostPrefixUpdate: once an Update gives up, here a +// prefix update, where a new prefix applies is unknown, so NAMESPACE_DONEs are +// no longer checked rather than risking a close on a conforming peer. +func TestNamespaceSubscriptionLostPrefixUpdate(t *testing.T) { + cli, srv := openPair(t) + sub, stream := namespaceSubscribePair(t, cli, srv) + b := sub.Broker() + go func() { _ = b.Serve(t.Context(), nil) }() + updated := make(chan struct{}) + go func() { + if _, err := message.Parse(stream); err != nil { + return + } + <-updated // answer only after the Update gave up + _ = message.Marshal(stream, &message.RequestOK{}) + _ = message.Marshal(stream, nsDone("b")) + }() + ctx, cancel := context.WithCancel(t.Context()) + cancel() + _, _ = sub.Update(ctx, message.Parameters{ + message.TrackNamespacePrefixParam(wire.TrackNamespace{[]byte("ns"), []byte("a")}), + }) + close(updated) + requireStaysOpen(t, cli, 100*time.Millisecond) +} + +// TestNamespaceSubscriptionLostUpdateShiftsPairing: once any Update gives up, +// its late answer pairs with the next update, so a prefix switch could land a +// response early. The check then stops rather than resolve an old-prefix +// NAMESPACE_DONE against the new prefix and close on a conforming peer. +func TestNamespaceSubscriptionLostUpdateShiftsPairing(t *testing.T) { + cli, srv := openPair(t) + sub, stream := namespaceSubscribePair(t, cli, srv) // prefix {ns} + b := sub.Broker() + go func() { _ = b.Serve(t.Context(), nil) }() + + ab := wire.TrackNamespace{[]byte("a"), []byte("b")} + gaveUp := make(chan struct{}) + peer := make(chan struct{}) + go func() { + defer close(peer) + if message.Marshal(stream, &message.Namespace{TrackNamespaceSuffix: ab}) != nil { + return + } + for range 2 { // the abandoned update, then the prefix update + if _, err := message.Parse(stream); err != nil { + return + } + } + <-gaveUp + _ = message.Marshal(stream, &message.RequestOK{}) // the first update's + _ = message.Marshal(stream, &message.NamespaceDone{TrackNamespaceSuffix: ab}) // old prefix + _ = message.Marshal(stream, &message.RequestOK{}) // the prefix update's + }() + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + _, _ = sub.Update(ctx, nil) + close(gaveUp) + ctx2, cancel2 := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel2() + _, _ = sub.Update(ctx2, message.Parameters{ + message.TrackNamespacePrefixParam(wire.TrackNamespace{[]byte("ns"), []byte("a")}), + }) + <-peer + requireStaysOpen(t, cli, 100*time.Millisecond) +} diff --git a/pkg/moqt/session/param_scope_test.go b/pkg/moqt/session/param_scope_test.go index 67dc9176..c9aaa1da 100644 --- a/pkg/moqt/session/param_scope_test.go +++ b/pkg/moqt/session/param_scope_test.go @@ -1,9 +1,11 @@ package session_test import ( + "math" "testing" "time" + "github.com/floatdrop/moq-go/pkg/moqt" "github.com/floatdrop/moq-go/pkg/moqt/message" "github.com/floatdrop/moq-go/pkg/moqt/session" ) @@ -248,3 +250,99 @@ func TestIncludePropertiesOutOfRangeCloses(t *testing.T) { }) } } + +// TestParamValueOutOfRangeCloses: a value the draft makes session-fatal closes +// the receiver, in an opener and inside its FILL_PARAMETERS: GROUP_ORDER +// outside {1, 2} (§10.2.8), FORWARD above 1 (§10.2.18), and a LOCATION_FILTER +// whose end Group overflows (§5.1.2) are PROTOCOL_VIOLATION; a LOCATION_FILTER +// or FILL_PARAMETERS that does not parse is KEY_VALUE_FORMATTING_ERROR (§1.4.3). +func TestParamValueOutOfRangeCloses(t *testing.T) { + t.Parallel() + overflow := message.AbsoluteRangeFilter(message.Location{Group: math.MaxUint64}, 1) + unparsable := message.BytesParam(message.ParamLocationFilter, []byte{0xFF}) + fill := func(inner ...message.Parameter) message.Parameter { return message.FillParametersParam(inner) } + cases := []struct { + name string + msg message.WithRequestID + want moqt.SessionErrorCode + }{ + {"GROUP_ORDER 0 in SUBSCRIBE", &message.Subscribe{ + Name: []byte("t"), + Parameters: message.Parameters{ + message.ByteParam(message.ParamGroupOrder, 0), + }, + }, moqt.SessionProtocolViolation}, + {"GROUP_ORDER 3 in PUBLISH", &message.Publish{ + Name: []byte("t"), + TrackAlias: 1, + Parameters: message.Parameters{ + message.ByteParam(message.ParamGroupOrder, 3), + }, + }, moqt.SessionProtocolViolation}, + {"GROUP_ORDER 5 in FETCH", &message.Fetch{ + Name: []byte("t"), + Parameters: message.Parameters{ + message.ByteParam(message.ParamGroupOrder, 5), + }, + }, moqt.SessionProtocolViolation}, + {"GROUP_ORDER 7 inside FILL_PARAMETERS", &message.Subscribe{ + Name: []byte("t"), + Parameters: message.Parameters{ + fill(message.ByteParam(message.ParamGroupOrder, 7)), + }, + }, moqt.SessionProtocolViolation}, + {"FORWARD 2 in PUBLISH", &message.Publish{Name: []byte("t"), TrackAlias: 1, + Parameters: message.Parameters{message.ByteParam(message.ParamForward, 2)}}, moqt.SessionProtocolViolation}, + {"LOCATION_FILTER overflow in SUBSCRIBE", &message.Subscribe{Name: []byte("t"), + Parameters: message.Parameters{overflow}}, moqt.SessionProtocolViolation}, + {"LOCATION_FILTER overflow in FETCH", &message.Fetch{Name: []byte("t"), + Parameters: message.Parameters{overflow}}, moqt.SessionProtocolViolation}, + {"LOCATION_FILTER overflow inside FILL_PARAMETERS", &message.SubscribeTracks{TrackNamespacePrefix: videoNS, + Parameters: message.Parameters{fill(overflow)}}, moqt.SessionProtocolViolation}, + {"LOCATION_FILTER that does not parse", &message.Subscribe{Name: []byte("t"), + Parameters: message.Parameters{unparsable}}, moqt.SessionKeyValueFormattingError}, + {"unknown parameter inside FILL_PARAMETERS", &message.Subscribe{Name: []byte("t"), + Parameters: message.Parameters{fill(message.VarintParam(0x3E, 1))}}, moqt.SessionProtocolViolation}, + {"FILL_PARAMETERS that does not parse", &message.Subscribe{Name: []byte("t"), + Parameters: message.Parameters{message.BytesParam(message.ParamFillParameters, []byte{0x05})}}, + moqt.SessionKeyValueFormattingError}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + client, server := openPair(t) + go func() { + tc.msg.SetRequestID(0) + _, _ = session.OpenRequestForTest(client, tc.msg) + }() + _, _ = server.AcceptRequest(t.Context()) + requireClosedCode(t, server, tc.want) + }) + } +} + +// TestParamValueOutOfRangeInFollowupsCloses: the same holds for the +// follow-ups a broker reads: FORWARD in PUBLISH_STATE_NOTIFY and in a +// REQUEST_UPDATE (§10.2.18). +func TestParamValueOutOfRangeInFollowupsCloses(t *testing.T) { + t.Parallel() + bad := message.Parameters{message.ByteParam(message.ParamForward, 3)} + t.Run("PUBLISH_STATE_NOTIFY", func(t *testing.T) { + t.Parallel() + client, server := openPair(t) + sub, pub := subscribePair(t, client, server) + go func() { _ = message.Marshal(pub.Stream, &message.PublishStateNotify{Parameters: bad}) }() + go func() { _ = sub.Broker().Serve(t.Context(), nil) }() + requireClosedProtocolViolation(t, client) + }) + t.Run("REQUEST_UPDATE", func(t *testing.T) { + t.Parallel() + client, server := openPair(t) + sub, pub := subscribePair(t, client, server) + go func() { + _ = message.Marshal(sub.Stream, &message.RequestUpdate{RequestID: client.AllocRequestID(), Parameters: bad}) + }() + go func() { _ = pub.Broker().Serve(t.Context(), nil) }() + requireClosedProtocolViolation(t, server) + }) +} diff --git a/pkg/moqt/session/publish.go b/pkg/moqt/session/publish.go index d197d026..fe464ee4 100644 --- a/pkg/moqt/session/publish.go +++ b/pkg/moqt/session/publish.go @@ -211,6 +211,9 @@ func (p *Publication) Done(code moqt.PublishDoneCode, reason string) error { // stays open for follow-ups — PUBLISH_DONE from the publisher, or a // REQUEST_UPDATE this side sends via [IncomingPublication.Update] to adjust // forwarding (§10.9). Close it to end the reception. +// +// Its Track Alias is released (§11.1) when the subscription is Terminated +// (§5.1), as for a [Subscription]. type IncomingPublication struct { // requestHandle carries the PUBLISH request stream — still open for // follow-up traffic (inbound PUBLISH_DONE, outbound REQUEST_UPDATE; @@ -247,7 +250,9 @@ func (s *Session) Publish(ctx context.Context, m *message.Publish) (*Publication func(stream Stream, _ *message.RequestOK) (*Publication, error) { // The PUBLISH sets the initial Forward State (§5.1); PUBLISH_OK // carries no subscription parameters. - return newPublication(s, stream, m.RequestID, m.TrackAlias, m.Parameters), nil + p := newPublication(s, stream, m.RequestID, m.TrackAlias, m.Parameters) + p.answered = message.TypePublish + return p, nil }) } @@ -288,7 +293,7 @@ func (s *Session) AwaitPublishOK(ctx context.Context, stream Stream) (*message.R } return m, nil case *message.RequestError: - return nil, &RequestRejectedError{Code: m.ErrorCode, Reason: m.ErrorReason, RetryInterval: m.RetryInterval} + return nil, s.rejection(m, message.TypePublish) default: return nil, fmt.Errorf("moqt/session: unexpected %s in PUBLISH response", resp.Type()) } diff --git a/pkg/moqt/session/redirect_test.go b/pkg/moqt/session/redirect_test.go new file mode 100644 index 00000000..94356d7f --- /dev/null +++ b/pkg/moqt/session/redirect_test.go @@ -0,0 +1,229 @@ +package session_test + +import ( + "errors" + "reflect" + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt" + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/session" + "github.com/floatdrop/moq-go/pkg/moqt/wire" +) + +// redirectError is a REDIRECT REQUEST_ERROR carrying rd. +func redirectError(rd message.Redirect) *message.RequestError { + return &message.RequestError{ErrorCode: moqt.RequestRedirect, ErrorReason: "elsewhere", Redirect: &rd} +} + +// answerRaw has sess accept the next request and write resp on its stream as +// is, bypassing the checks [session.Request.Reject] applies. +func answerRaw(t *testing.T, sess *session.Session, resp message.Message) { + t.Helper() + go func() { + r, err := sess.AcceptRequest(t.Context()) + if err != nil { + return + } + _ = message.Marshal(r.Stream, resp) + }() +} + +// TestRedirectFollowable: a REDIRECT's Redirect reaches the requester on its +// RequestRejectedError (§10.6.1), sent with Reject, and a Connect URI to the +// client or a Track Name for a SUBSCRIBE keeps the session open. +func TestRedirectFollowable(t *testing.T) { + cli, srv := openPair(t) + want := &message.Redirect{ + ConnectURI: []byte("https://other.example/moq"), + Namespace: wire.TrackNamespace{[]byte("elsewhere")}, + TrackName: []byte("t2"), + } + rejected := make(chan error, 1) + go func() { + r, err := srv.AcceptRequest(t.Context()) + if err != nil { + rejected <- err + return + } + rejected <- r.Reject(&session.RequestRejectedError{Code: moqt.RequestRedirect, Reason: "moved", Redirect: want}) + }() + _, err := cli.Subscribe(t.Context(), &message.Subscribe{Name: []byte("t")}) + if err := <-rejected; err != nil { + t.Fatalf("Reject: %v", err) + } + rej, ok := errors.AsType[*session.RequestRejectedError](err) + if !ok || rej.Code != moqt.RequestRedirect { + t.Fatalf("Subscribe = %v, want a REDIRECT", err) + } + if !reflect.DeepEqual(rej.Redirect, want) { + t.Fatalf("Redirect = %+v, want %+v", rej.Redirect, want) + } + requireStaysOpen(t, cli, 50*time.Millisecond) +} + +// TestReceivedRedirectViolationCloses: a Redirect with a Connect URI received +// by a server, or with a Track Name for a namespace-scoped request, closes the +// session with PROTOCOL_VIOLATION (§10.6.1), on every path a REQUEST_ERROR is +// read. +func TestReceivedRedirectViolationCloses(t *testing.T) { + uri := message.Redirect{ConnectURI: []byte("https://other.example/moq")} + name := message.Redirect{Namespace: wire.TrackNamespace{[]byte("ns")}, TrackName: []byte("t")} + ns := wire.TrackNamespace{[]byte("ns")} + for _, tc := range []struct { + name string + // toServer: the server is the requester and receives the REDIRECT. + toServer bool + rd message.Redirect + open func(t *testing.T, requester *session.Session) error + }{ + {"Connect URI at the server, SUBSCRIBE", true, uri, func(t *testing.T, s *session.Session) error { + _, err := s.Subscribe(t.Context(), &message.Subscribe{Name: []byte("t")}) + return err + }}, + {"Connect URI at the server, AwaitPublishOK", true, uri, func(t *testing.T, s *session.Session) error { + stream, err := s.OpenPublish(&message.Publish{Name: []byte("t"), TrackAlias: 1}) + if err != nil { + return err + } + _, err = s.AwaitPublishOK(t.Context(), stream) + return err + }}, + {"Track Name for SUBSCRIBE_NAMESPACE", false, name, func(t *testing.T, s *session.Session) error { + _, err := s.SubscribeNamespace(t.Context(), &message.SubscribeNamespace{TrackNamespacePrefix: ns}) + return err + }}, + {"Track Name for PUBLISH_NAMESPACE", false, name, func(t *testing.T, s *session.Session) error { + _, err := s.PublishNamespace(t.Context(), &message.PublishNamespace{Namespace: ns}) + return err + }}, + {"Track Name for SUBSCRIBE_TRACKS", false, name, func(t *testing.T, s *session.Session) error { + _, err := s.SubscribeTracks(t.Context(), &message.SubscribeTracks{TrackNamespacePrefix: ns}) + return err + }}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + requester, responder := cli, srv + if tc.toServer { + requester, responder = srv, cli + } + answerRaw(t, responder, redirectError(tc.rd)) + _ = tc.open(t, requester) + requireClosedCode(t, requester, moqt.SessionProtocolViolation) + }) + } +} + +// TestReceivedRedirectViolationClosesOnUpdate: a Redirect carrying a Connect +// URI on a server's request stream after the response closes the session +// (§10.6.1): answering a REQUEST_UPDATE read by Update itself or routed by the +// broker, or unsolicited, handed to Serve's callback. +func TestReceivedRedirectViolationClosesOnUpdate(t *testing.T) { + for _, tc := range []struct { + name string + served, apply bool + }{ + {"read by Update", false, true}, + {"routed by the broker", true, true}, + {"unsolicited", true, false}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + sub, pub := subscribePair(t, srv, cli) + go func() { + if tc.apply { + if _, err := message.Parse(pub); err != nil { + return + } + } + _ = message.Marshal( + pub, + redirectError(message.Redirect{ConnectURI: []byte("https://other.example/moq")}), + ) + }() + if tc.served { + b := sub.Broker() + go func() { _ = b.Serve(t.Context(), nil) }() + } + if tc.apply { + _, _ = sub.Update(t.Context(), nil) + } + requireClosedCode(t, srv, moqt.SessionProtocolViolation) + }) + } +} + +// TestRejectRefusesSessionFatalRedirect: Reject does not send a Redirect its +// peer would have to close the session for (§10.6.1), so the request can still +// be refused another way. +func TestRejectRefusesSessionFatalRedirect(t *testing.T) { + ns := wire.TrackNamespace{[]byte("ns")} + for _, tc := range []struct { + name string + toServer bool + rd *message.Redirect + open func(t *testing.T, requester *session.Session) error + }{ + {"Connect URI to the server", true, &message.Redirect{ConnectURI: []byte("https://other.example/moq")}, + func(t *testing.T, s *session.Session) error { + _, err := s.Subscribe(t.Context(), &message.Subscribe{Name: []byte("t")}) + return err + }}, + {"Track Name for SUBSCRIBE_NAMESPACE", false, &message.Redirect{Namespace: ns, TrackName: []byte("t")}, + func(t *testing.T, s *session.Session) error { + _, err := s.SubscribeNamespace(t.Context(), &message.SubscribeNamespace{TrackNamespacePrefix: ns}) + return err + }}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + requester, responder := cli, srv + if tc.toServer { + requester, responder = srv, cli + } + refused := make(chan error, 1) + go func() { + r, err := responder.AcceptRequest(t.Context()) + if err != nil { + refused <- err + return + } + refused <- r.Reject(&session.RequestRejectedError{Code: moqt.RequestRedirect, Redirect: tc.rd}) + _ = r.RejectError(moqt.RequestDoesNotExist, "no") + }() + err := tc.open(t, requester) + if rej, ok := errors.AsType[*session.RequestRejectedError]( + err, + ); !ok || + rej.Code != moqt.RequestDoesNotExist { + t.Fatalf("request = %v, want the DOES_NOT_EXIST sent after the refused REDIRECT", err) + } + if err := <-refused; err == nil { + t.Fatal("Reject sent a Redirect the peer must close the session for") + } + requireStaysOpen(t, requester, 50*time.Millisecond) + }) + } +} + +// TestUpdateHandlerRedirectSentAsInternalError: REDIRECT cannot answer a +// REQUEST_UPDATE (§10.6.2), so an UpdateHandler's REDIRECT goes out as +// INTERNAL_ERROR rather than as a REDIRECT the peer cannot parse, and both +// sessions stay open. +func TestUpdateHandlerRedirectSentAsInternalError(t *testing.T) { + client, server := openPair(t) + sub, pub := subscribePair(t, client, server) + b := pub.Broker() + b.HandleUpdates(func(*message.RequestUpdate) (*message.RequestOK, error) { + return nil, &session.RequestRejectedError{Code: moqt.RequestRedirect, Reason: "moved"} + }) + go func() { _ = b.Serve(t.Context(), nil) }() + + _, err := sub.Update(t.Context(), message.Parameters{message.ForwardParam(false)}) + if rej, ok := errors.AsType[*session.RequestRejectedError](err); !ok || rej.Code != moqt.RequestInternalError { + t.Fatalf("Update = %v, want INTERNAL_ERROR", err) + } + requireStaysOpen(t, client, 50*time.Millisecond) +} diff --git a/pkg/moqt/session/request.go b/pkg/moqt/session/request.go index c5927d86..535e4d76 100644 --- a/pkg/moqt/session/request.go +++ b/pkg/moqt/session/request.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "io" "math" "slices" "sync" @@ -13,6 +14,7 @@ import ( "github.com/floatdrop/moq-go/pkg/moqt" "github.com/floatdrop/moq-go/pkg/moqt/message" "github.com/floatdrop/moq-go/pkg/moqt/track" + "github.com/floatdrop/moq-go/pkg/moqt/wire" ) // ErrRequestIDParityViolation is returned by [Session.CheckPeerRequestID] when @@ -169,6 +171,44 @@ type RequestRejectedError struct { // RetryInterval is the raw Retry Interval (§10.6.2); see // [RequestRejectedError.RetryAfter]. RetryInterval uint64 + // Redirect is where to retry the request (§10.6.1), present exactly when + // Code is REDIRECT: an empty ConnectURI means this session's URI. + Redirect *message.Redirect +} + +// rejection converts a REQUEST_ERROR answering a message of type req into a +// *RequestRejectedError; for a REQUEST_UPDATE the request it updates is not +// known, so only the Connect URI is checked. A Redirect that §10.6.1 makes +// session-fatal closes the session with PROTOCOL_VIOLATION, and that error is +// returned instead. +func (s *Session) rejection(m *message.RequestError, req message.Type) error { + if err := redirectViolation(m.Redirect, req, s.role == roleServer); err != nil { + return s.closeProtocolViolation(fmt.Errorf("moqt/session: received %w", err)) + } + return &RequestRejectedError{ + Code: m.ErrorCode, + Reason: m.ErrorReason, + RetryInterval: m.RetryInterval, + Redirect: m.Redirect, + } +} + +// redirectViolation reports a Redirect for a request of type req that the +// receiver, a server when atServer, MUST close the session for (§10.6.1): "If +// a server receives a Redirect with a non-zero Connect URI Length", or a +// non-empty Track Name "in a Redirect for a namespace-scoped request". +func redirectViolation(rd *message.Redirect, req message.Type, atServer bool) error { + switch { + case rd == nil: + return nil + case atServer && len(rd.ConnectURI) > 0: + return errors.New("a Redirect with a Connect URI at a server (§10.6.1)") + case len(rd.TrackName) > 0 && + (req == message.TypeSubscribeNamespace || req == message.TypePublishNamespace || + req == message.TypeSubscribeTracks): + return fmt.Errorf("a Redirect with a Track Name for %s (§10.6.1)", req) + } + return nil } // RetryAfter decodes RetryInterval (§10.6.2): whether the request may be @@ -237,9 +277,10 @@ type Request struct { // requests for session-level tracks and namespaces". AcceptRequest loops until // it has an application-visible request to return. // -// A stream not opened by a request message (§3.3), or whose first message is -// malformed (§10, wrapping [message.ErrMalformedMessage]), closes the session -// with PROTOCOL_VIOLATION; the error is *ErrUnexpectedRequestOpener, +// A stream not opened by a request message (§3.3), whose first message is +// malformed (§10, wrapping [message.ErrMalformedMessage]), or a PUBLISH with a +// session-fatal Track Property value (§12.5, §12.6), closes the session with +// PROTOCOL_VIOLATION; the error is *ErrUnexpectedRequestOpener, // *ErrUnexpectedRequestUpdate, ErrUnexpectedPublishStateNotify or the parse // error. A Request ID violation (§10.1) closes it with INVALID_REQUEST_ID and // returns *ErrRequestIDParityViolation or *ErrDuplicateRequestID, and a token @@ -303,6 +344,13 @@ func (s *Session) AcceptRequest(ctx context.Context) (*Request, error) { resetStream(stream) return nil, err } + // §12.5, §12.6: on receipt, whatever the application answers. + if pub, ok := msg.(*message.Publish); ok { + if err := s.checkTrackPropertyValues(pub.TrackProperties, "PUBLISH"); err != nil { + resetStream(stream) + return nil, err + } + } // §10.1 parity and duplicate check. if m, ok := msg.(message.WithRequestID); ok { @@ -476,15 +524,77 @@ type requestHandle struct { // its REQUEST_UPDATEs; applied to the broker on creation. peerUpdate, peerNotify bool updateScope message.ParamScope + + // inboundAlias is the inbound Track Alias this request registered + // (§11.1), when holdsAlias: the receiving side of a subscription. It is + // released once the subscription is Terminated (§5.1); see terminated. + inboundAlias uint64 + holdsAlias bool + released atomic.Bool + // peerDone records that the publisher's FIN was read. + peerDone atomic.Bool + + // namespaces marks a SUBSCRIBE_NAMESPACE, whose broker checks each + // NAMESPACE_DONE against the NAMESPACEs before it (§10.19); nsPrefix is + // the Track Namespace Prefix it was sent with. + namespaces bool + nsPrefix wire.TrackNamespace + + // goaways checks the GOAWAYs this stream carries (§10.4), for every + // reader of the handle. + goaways RequestGoaways + + // answered is TypeSubscribe or TypePublish for a request this side sent, + // whose response was read: its broker closes the session on another + // (§5.1). Zero otherwise. + answered message.Type +} + +// Read reads the request stream. On the receiving side of a subscription +// ([Subscription], [IncomingPublication]) the publisher's FIN ends it — "by +// sending PUBLISH_DONE and closing the stream" (§5.1) — which releases the +// Track Alias (§11.1). +func (h *requestHandle) Read(p []byte) (int, error) { + n, err := h.Stream.Read(p) + if errors.Is(err, io.EOF) { + h.peerFinished() + } + return n, err +} + +// peerFinished records the publisher's FIN on the receiving side of a +// subscription, which Terminates it (§5.1). +func (h *requestHandle) peerFinished() { + if h.holdsAlias { + h.peerDone.Store(true) + h.terminated() + } +} + +// terminated releases the inbound Track Alias the first time the subscription +// is Terminated: a handle can see that more than once (Close after the FIN), +// and a second release would take a registration another subscription +// sharing the alias holds (§5.1). +func (h *requestHandle) terminated() { + if h.holdsAlias && h.released.CompareAndSwap(false, true) { + h.s.UnregisterInboundTrackAlias(h.inboundAlias) + } } // Close cancels the request by resetting both stream directions (§3.3.3). If // this side already sent its final message and FIN (e.g. [Publication.Done]), -// only reading is stopped, so that message is not lost. +// only reading is stopped, so that message is not lost. A subscription is then +// Terminated (§5.1), so its Track Alias is released. // -// Close does not know whether the peer already completed the request; after -// that, §3.3.2 says the requester SHOULD FIN, so use Stream.Close instead. +// Once a subscription's publisher has completed it (its FIN was read through +// the handle or its broker), Close FINs instead, as §3.3.2 asks once nothing +// further will be sent. Otherwise Close cannot know whether the peer completed the +// request; after that, use Stream.Close to FIN. func (h *requestHandle) Close() error { + h.terminated() + if h.peerDone.Load() { + return h.Stream.Close() + } if h.finished.Load() { h.Stream.CancelRead(uint64(moqt.StreamResetCancelled)) return nil @@ -508,6 +618,8 @@ func (h *requestHandle) Broker() *RequestBroker { b := h.s.NewRequestBroker(h.Stream) b.PeerMessages(h.peerUpdate, h.peerNotify) b.UpdateScope(h.updateScope) + b.handle = h + b.nsPrefix = h.nsPrefix h.broker.Store(b) }) return h.broker.Load() @@ -522,7 +634,7 @@ func (h *requestHandle) Update(ctx context.Context, params message.Parameters) ( if b := h.broker.Load(); b != nil { return b.Update(ctx, params) } - return h.s.UpdateRequest(ctx, h.Stream, params) + return h.s.UpdateRequest(ctx, h, params) // h, so Read sees a FIN } // writeThenClose writes msg and FINs the send side, through the broker's @@ -602,8 +714,9 @@ func (s *Session) readResponse(ctx context.Context, stream Stream) (message.Mess // awaitRequestResponse opens a request stream for m and awaits the initial // response. An OK is handed to onOK, which then owns the stream; REQUEST_ERROR -// (§10.6) becomes a *RequestRejectedError; anything else is an error. On -// either failure the stream is closed. +// (§10.6) becomes a *RequestRejectedError; anything else is an error, and for +// SUBSCRIBE_NAMESPACE and SUBSCRIBE_TRACKS also closes the session (§10.19, +// §10.20). On either failure the stream is closed. func awaitRequestResponse[OK message.Message, R any]( ctx context.Context, s *Session, @@ -638,13 +751,16 @@ func awaitRequestResponse[OK message.Message, R any]( } _ = stream.Close() if rerr, isErr := resp.(*message.RequestError); isErr { - return zero, &RequestRejectedError{ - Code: rerr.ErrorCode, - Reason: rerr.ErrorReason, - RetryInterval: rerr.RetryInterval, - } + return zero, s.rejection(rerr, m.Type()) + } + err = fmt.Errorf("moqt/session: unexpected %s in %s response", resp.Type(), m.Type()) + // §10.19, §10.20: "If the subscriber receives any message other than a + // REQUEST_OK or a REQUEST_ERROR as the first message on the response half + // of the stream, then it MUST close the session with a PROTOCOL_VIOLATION." + if t := m.Type(); t == message.TypeSubscribeNamespace || t == message.TypeSubscribeTracks { + return zero, s.closeProtocolViolation(err) } - return zero, fmt.Errorf("moqt/session: unexpected %s in %s response", resp.Type(), m.Type()) + return zero, err } // UpdateRequest sends a REQUEST_UPDATE (§10.9) with a fresh Request ID (§10.1) @@ -674,8 +790,10 @@ func (s *Session) UpdateRequest( } // CheckPeerParams checks the Message Parameters of a peer message m against -// scope (§10.2.1) and §10.2's duplicate rule. On a violation it closes the -// session with PROTOCOL_VIOLATION and returns the error. +// scope (§10.2.1), §10.2's duplicate rule and the values the draft makes +// session-fatal (see [message.Parameters.CheckScope]). On a violation it +// closes the session, with KEY_VALUE_FORMATTING_ERROR for a value that does +// not parse (§1.4.3) and PROTOCOL_VIOLATION otherwise, and returns the error. // // The session checks the messages it reads itself; callers that read a // request stream with [message.Parse] call it for what they read. @@ -685,6 +803,10 @@ func (s *Session) CheckPeerParams(scope message.ParamScope, m message.Message) e return nil } if err := params.CheckScope(scope); err != nil { + if errors.Is(err, message.ErrValueFormatting) { + _ = s.Close(moqt.SessionKeyValueFormattingError, err.Error()) + return err + } return s.closeProtocolViolation(err) } return nil @@ -760,18 +882,28 @@ func (r *Request) RejectError(code moqt.RequestErrorCode, reason string) error { return r.Reject(&RequestRejectedError{Code: code, Reason: reason}) } -// Reject is [Request.RejectError] with rej's Code, Reason and RetryInterval -// (§10.6.2). REDIRECT is refused and nothing is written, since Reject has no -// Redirect structure to send. +// Reject is [Request.RejectError] with rej's Code, Reason, RetryInterval and, +// for REDIRECT, Redirect (§10.6.2). Nothing is written, and an error returned, +// when the Redirect does not match the code, or is one the peer MUST close the +// session for (§10.6.1): a Connect URI sent to a server, or a Track Name for a +// namespace-scoped request. func (r *Request) Reject(rej *RequestRejectedError) error { - if rej.Code == moqt.RequestRedirect { - return errors.New("moqt/session: Reject cannot send REDIRECT: it has no Redirect structure (§10.6.2)") - } - if err := message.Marshal(r.Stream, &message.RequestError{ + m := &message.RequestError{ ErrorCode: rej.Code, RetryInterval: rej.RetryInterval, ErrorReason: rej.Reason, - }); err != nil { + Redirect: rej.Redirect, + } + if err := m.ValidateRedirect(); err != nil { + return fmt.Errorf("moqt/session: Reject: %w", err) + } + // Refuse what the peer would have to close the session for. + if rej.Redirect != nil { + if err := redirectViolation(rej.Redirect, r.First.Type(), r.s.role == roleClient); err != nil { + return fmt.Errorf("moqt/session: Reject: %w", err) + } + } + if err := message.Marshal(r.Stream, m); err != nil { resetStream(r.Stream) return err } @@ -784,19 +916,12 @@ func (r *Request) Reject(rej *RequestRejectedError) error { // MUST be a *message.Subscribe. // // ok may be nil for the all-default reply; a zero TrackAlias is allocated with -// [Session.AllocOutboundTrackAlias]. A FORWARD value above 1 closes the -// session with PROTOCOL_VIOLATION (§10.2.18). +// [Session.AllocOutboundTrackAlias]. func (r *Request) AcceptSubscribe(ok *message.SubscribeOK) (*Publication, error) { sub, isSub := r.First.(*message.Subscribe) if !isSub { return nil, fmt.Errorf("moqt/session: AcceptSubscribe on a %s request", r.First.Type()) } - // §10.2.18. - if f, found := sub.Parameters.Find(message.ParamForward); found && f.Byte > 1 { - resetStream(r.Stream) - return nil, r.s.closeProtocolViolation( - fmt.Errorf("moqt/session: FORWARD value %d in SUBSCRIBE", f.Byte)) - } if ok == nil { ok = &message.SubscribeOK{} } @@ -816,10 +941,10 @@ func (r *Request) AcceptSubscribe(ok *message.SubscribeOK) (*Publication, error) // Track Properties that fail validation (see // [WithKnownMandatoryTrackProperties]) are rejected with REQUEST_ERROR — // UNSUPPORTED_EXTENSION for an unknown Mandatory Track Property (§2.5.1), -// MALFORMED_TRACK for ones that do not parse — and the error returned. On an -// alias collision *ErrDuplicateTrackAlias is returned without replying; the -// caller MUST close the session with [moqt.SessionDuplicateTrackAlias] -// (§11.1). +// MALFORMED_TRACK for ones that do not parse — and the error returned. A +// session-fatal value (§12.5, §12.6) closed the session in AcceptRequest. An +// alias collision closes the session with DUPLICATE_TRACK_ALIAS and returns +// *ErrDuplicateTrackAlias (§11.1). func (r *Request) AcceptPublish() (*IncomingPublication, error) { pub, isPub := r.First.(*message.Publish) if !isPub { @@ -834,17 +959,21 @@ func (r *Request) AcceptPublish() (*IncomingPublication, error) { return nil, err } if err := message.Marshal(r.Stream, &message.RequestOK{}); err != nil { + // Never Established (§5.1), so the alias is not held (§11.1). + r.s.UnregisterInboundTrackAlias(pub.TrackAlias) return nil, fmt.Errorf("moqt/session: write PUBLISH REQUEST_OK: %w", err) } // The publisher may send REQUEST_UPDATE (§10.9) and PUBLISH_STATE_NOTIFY // (§10.10). return &IncomingPublication{ - Stream: r.Stream, - s: r.s, - requestID: pub.RequestID, - peerUpdate: true, - peerNotify: true, - updateScope: message.ScopeUpdateFromPublisher, - alias: pub.TrackAlias, + Stream: r.Stream, + s: r.s, + requestID: pub.RequestID, + peerUpdate: true, + peerNotify: true, + updateScope: message.ScopeUpdateFromPublisher, + inboundAlias: pub.TrackAlias, + holdsAlias: true, + alias: pub.TrackAlias, }, nil } diff --git a/pkg/moqt/session/request_goaway_test.go b/pkg/moqt/session/request_goaway_test.go new file mode 100644 index 00000000..af5137f4 --- /dev/null +++ b/pkg/moqt/session/request_goaway_test.go @@ -0,0 +1,84 @@ +package session_test + +import ( + "context" + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt" + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/session" +) + +// TestRequestStreamGoawayViolationCloses: a second GOAWAY on one request +// stream, or a GOAWAY carrying a New Session URI received by the server, +// closes the session with PROTOCOL_VIOLATION (§10.4), read by the receiving +// handle's broker. +func TestRequestStreamGoawayViolationCloses(t *testing.T) { + uri := &message.Goaway{NewSessionURI: []byte("https://relay.example/moq")} + for _, tc := range []struct { + name string + toServer bool + sent []*message.Goaway + }{ + {"second GOAWAY to the client", false, []*message.Goaway{{}, uri}}, + {"second GOAWAY to the server", true, []*message.Goaway{{}, {}}}, + {"New Session URI to the server", true, []*message.Goaway{uri}}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + sub, pub := subscribePair(t, cli, srv) + var from session.Stream = pub + receiver, broker := cli, sub.Broker() + if tc.toServer { + from = sub + receiver, broker = srv, pub.Broker() + } + go func() { + for _, m := range tc.sent { + if err := message.Marshal(from, m); err != nil { + return + } + } + }() + // Bounded: a GOAWAY that is ignored leaves the stream open. + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel() + _ = broker.Serve(ctx, nil) + requireClosedCode(t, receiver, moqt.SessionProtocolViolation) + }) + } +} + +// TestRequestStreamGoawayOnceIsDelivered: one GOAWAY on a request stream, with +// a New Session URI when sent to the client, is legal (§10.4) and reaches +// Serve's callback. +func TestRequestStreamGoawayOnceIsDelivered(t *testing.T) { + cli, srv := openPair(t) + sub, pub := subscribePair(t, cli, srv) + go func() { + if err := message.Marshal( + pub, + &message.Goaway{NewSessionURI: []byte("https://relay.example/moq")}, + ); err != nil { + return + } + _ = pub.Done(moqt.PublishDoneGoingAway, "") + }() + var got []message.Message + if err := sub.Broker().Serve(t.Context(), func(m message.Message) bool { + got = append(got, m) + return true + }); err != nil { + t.Fatalf("Serve: %v", err) + } + if len(got) == 0 { + t.Fatal("Serve's callback never saw the GOAWAY") + } + if _, ok := got[0].(*message.Goaway); !ok { + t.Fatalf("first message = %T, want *message.Goaway", got[0]) + } + if err := cli.Err(); err != nil { + t.Fatalf("one GOAWAY on a request stream closed the session: %v", err) + } +} diff --git a/pkg/moqt/session/request_reject_test.go b/pkg/moqt/session/request_reject_test.go index b0e0283e..a96ec9d9 100644 --- a/pkg/moqt/session/request_reject_test.go +++ b/pkg/moqt/session/request_reject_test.go @@ -100,9 +100,9 @@ func TestRejectSendsRetryInterval(t *testing.T) { } } -// TestRejectRefusesRedirect: a REDIRECT needs a Redirect structure (§10.6.2) -// that RequestRejectedError cannot carry, so Reject refuses without writing -// and the request can still be refused another way. +// TestRejectRefusesRedirect: a REDIRECT needs a Redirect structure (§10.6.2), +// so Reject refuses one without it and writes nothing, and the request can +// still be refused another way. func TestRejectRefusesRedirect(t *testing.T) { client, server := openPair(t) refused := make(chan error, 1) diff --git a/pkg/moqt/session/second_response_test.go b/pkg/moqt/session/second_response_test.go new file mode 100644 index 00000000..c040008e --- /dev/null +++ b/pkg/moqt/session/second_response_test.go @@ -0,0 +1,125 @@ +package session_test + +import ( + "context" + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt" + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/session" +) + +// TestSecondResponseCloses: a second response to a SUBSCRIBE or PUBLISH read +// by the requester's broker closes the session with PROTOCOL_VIOLATION (§5.1: +// "The peer SHOULD close the session with a protocol error if it receives more +// than one"), as does a REQUEST_ERROR where no REQUEST_UPDATE was sent. +func TestSecondResponseCloses(t *testing.T) { + for _, tc := range []struct { + name string + resp message.Message + // viaPublish: the server PUBLISHes and the client answers twice; + // else the client SUBSCRIBEs and the server answers twice. + viaPublish bool + }{ + {"second SUBSCRIBE_OK", &message.SubscribeOK{TrackAlias: 1}, false}, + {"REQUEST_ERROR after SUBSCRIBE_OK", &message.RequestError{ErrorCode: moqt.RequestInternalError}, false}, + {"second PUBLISH_OK", &message.RequestOK{}, true}, + {"REQUEST_ERROR after PUBLISH_OK", &message.RequestError{ErrorCode: moqt.RequestInternalError}, true}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + requester := cli + var ( + from session.Stream + broker *session.RequestBroker + ) + if tc.viaPublish { + requester = srv + h, pub := establishOnAlias(t, cli, srv, true, "t", 7) + from, broker = h.(*session.IncomingPublication), pub.Broker() + } else { + sub, pub := subscribePair(t, cli, srv) + from, broker = pub, sub.Broker() + } + go func() { _ = message.Marshal(from, tc.resp) }() + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel() + _ = broker.Serve(ctx, nil) + requireClosedCode(t, requester, moqt.SessionProtocolViolation) + }) + } +} + +// TestResponderStreamResponseKeepsSession: on a stream this side answered, +// the peer is the requester, so a REQUEST_OK or REQUEST_ERROR from it is no +// second response (§5.1) and reaches Serve's callback. +func TestResponderStreamResponseKeepsSession(t *testing.T) { + cli, srv := openPair(t) + sub, pub := subscribePair(t, cli, srv) + go func() { _ = message.Marshal(sub, &message.RequestError{ErrorCode: moqt.RequestInternalError}) }() + got := make(chan message.Message, 1) + go func() { + _ = pub.Broker().Serve(t.Context(), func(m message.Message) bool { + got <- m + return false + }) + }() + select { + case m := <-got: + if _, ok := m.(*message.RequestError); !ok { + t.Fatalf("Serve's callback got %T, want *message.RequestError", m) + } + case <-time.After(2 * time.Second): + t.Fatal("the REQUEST_ERROR never reached Serve's callback") + } + requireStaysOpen(t, srv, 50*time.Millisecond) +} + +// TestSubscribeOKOnPublishKeepsSession: a SUBSCRIBE_OK is no response to a +// PUBLISH (§5.1), so on this side's PUBLISH it reaches Serve's callback. +func TestSubscribeOKOnPublishKeepsSession(t *testing.T) { + cli, srv := openPair(t) + h, pub := establishOnAlias(t, cli, srv, true, "t", 7) + go func() { _ = message.Marshal(h.(*session.IncomingPublication), &message.SubscribeOK{TrackAlias: 1}) }() + got := make(chan message.Message, 1) + go func() { + _ = pub.Broker().Serve(t.Context(), func(m message.Message) bool { + got <- m + return false + }) + }() + select { + case m := <-got: + if _, ok := m.(*message.SubscribeOK); !ok { + t.Fatalf("Serve's callback got %T, want *message.SubscribeOK", m) + } + case <-time.After(2 * time.Second): + t.Fatal("the SUBSCRIBE_OK never reached Serve's callback") + } + requireStaysOpen(t, srv, 50*time.Millisecond) +} + +// TestLateUpdateAnswerKeepsSession: the answer to a REQUEST_UPDATE whose +// Update gave up still answers a REQUEST_UPDATE that was sent (§10.9), so it +// does not read as a second response. +func TestLateUpdateAnswerKeepsSession(t *testing.T) { + cli, srv := openPair(t) + sub, pub := subscribePair(t, cli, srv) + b := sub.Broker() + go func() { _ = b.Serve(t.Context(), nil) }() + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + answered := make(chan struct{}) + go func() { + defer close(answered) + if _, err := message.Parse(pub); err != nil { + return + } + _ = message.Marshal(pub, &message.RequestOK{}) + }() + _, _ = b.Update(ctx, nil) + <-answered + requireStaysOpen(t, cli, 100*time.Millisecond) +} diff --git a/pkg/moqt/session/subscribe.go b/pkg/moqt/session/subscribe.go index 45b2282a..bc7ee607 100644 --- a/pkg/moqt/session/subscribe.go +++ b/pkg/moqt/session/subscribe.go @@ -13,6 +13,12 @@ import ( // publisher-assigned Track Alias — so the caller can send REQUEST_UPDATE via // [Subscription.Update] without holding them separately. It is returned by // [Session.Subscribe]. +// +// The Track Alias stays registered while the subscription is Established and +// is released (§11.1) once it is Terminated (§5.1): on [Subscription.Close] or +// its broker's [RequestBroker.Close], when the publisher's FIN is read through +// the Subscription or by its broker's [RequestBroker.Serve], or when Serve +// cancels the stream. type Subscription struct { // requestHandle carries the SUBSCRIBE request stream — still open for // follow-up traffic (REQUEST_UPDATE and inbound PUBLISH_DONE; Close it @@ -55,11 +61,14 @@ func (s *Session) Subscribe(ctx context.Context, m *message.Subscribe) (*Subscri // The publisher may send PUBLISH_STATE_NOTIFY (§10.10) but not // REQUEST_UPDATE (§10.9). return &Subscription{ - Stream: stream, - s: s, - requestID: m.RequestID, - peerNotify: true, - OK: ok, + Stream: stream, + s: s, + requestID: m.RequestID, + peerNotify: true, + inboundAlias: ok.TrackAlias, + holdsAlias: true, + answered: message.TypeSubscribe, + OK: ok, }, nil }) } diff --git a/pkg/moqt/session/subscribe_test.go b/pkg/moqt/session/subscribe_test.go index 1438e273..51e70457 100644 --- a/pkg/moqt/session/subscribe_test.go +++ b/pkg/moqt/session/subscribe_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "io" "sync" "testing" "time" @@ -237,8 +238,8 @@ func TestSubscribe_ContextCancelUnblocksResponseWait(t *testing.T) { // TestRegisterInboundTrackAlias verifies the Session.RegisterInboundTrackAlias // method directly: same alias + same track is counted (§5.1 lets concurrent // subscriptions to one Track share an alias), and stays registered until each -// registration is released; same alias + different track returns -// *ErrDuplicateTrackAlias. +// registration is released; same alias + different track closes the session +// with DUPLICATE_TRACK_ALIAS and returns *ErrDuplicateTrackAlias (§11.1). func TestRegisterInboundTrackAlias(t *testing.T) { cli, _ := openPair(t) @@ -255,16 +256,6 @@ func TestRegisterInboundTrackAlias(t *testing.T) { t.Fatalf("second register: %v", err) } - // Same alias, different track → ErrDuplicateTrackAlias. - err := cli.RegisterInboundTrackAlias(42, keyB) - var dupErr *session.ErrDuplicateTrackAlias - if !errors.As(err, &dupErr) { - t.Fatalf("register different track: error = %v (%T), want *session.ErrDuplicateTrackAlias", err, err) - } - if dupErr.Alias != 42 { - t.Errorf("Alias = %d, want 42", dupErr.Alias) - } - // Different alias, same track → fine (multiple aliases can point to the same track). if err := cli.RegisterInboundTrackAlias(99, keyA); err != nil { t.Fatalf("different alias same track: %v", err) @@ -286,64 +277,278 @@ func TestRegisterInboundTrackAlias(t *testing.T) { // Unregistering a non-existent alias is a no-op. cli.UnregisterInboundTrackAlias(12345) -} -// TestSubscribeDuplicateTrackAlias verifies that when the server assigns the -// same Track Alias to two different tracks via SUBSCRIBE_OK, the second -// Subscribe call returns *ErrDuplicateTrackAlias. -func TestSubscribeDuplicateTrackAlias(t *testing.T) { - cli, srv := openPair(t) - ctx := t.Context() + // Same alias, different track → DUPLICATE_TRACK_ALIAS. + err := cli.RegisterInboundTrackAlias(99, keyB) + dupErr, ok := errors.AsType[*session.ErrDuplicateTrackAlias](err) + if !ok { + t.Fatalf("register different track: error = %v (%T), want *session.ErrDuplicateTrackAlias", err, err) + } + if dupErr.Alias != 99 { + t.Errorf("Alias = %d, want 99", dupErr.Alias) + } + requireClosedCode(t, cli, moqt.SessionDuplicateTrackAlias) +} - const sharedAlias = uint64(7) +// TestDuplicateTrackAliasCloses: a SUBSCRIBE_OK or PUBLISH that reuses the +// Track Alias of a different Track with an Established subscription closes the +// session with DUPLICATE_TRACK_ALIAS (§11.1), whichever message bound it +// first. +func TestDuplicateTrackAliasCloses(t *testing.T) { + const alias = uint64(7) + ns := wire.TrackNamespace{[]byte("ns")} - // Helper: run one subscribe round-trip. - doSubscribe := func(ns wire.TrackNamespace, name []byte) error { - var ( - wg sync.WaitGroup - srvErr error - cliErr error - ) - wg.Go(func() { - r, err := srv.AcceptRequest(ctx) - if err != nil { - srvErr = err - return + // subscribe has srv answer cli's SUBSCRIBE to name with alias, and + // returns cli's result. + subscribe := func(t *testing.T, cli, srv *session.Session, name string) error { + t.Helper() + go func() { + if r, err := srv.AcceptRequest(t.Context()); err == nil { + _ = r.Reply(&message.SubscribeOK{TrackAlias: alias}) } - srvErr = r.Reply(&message.SubscribeOK{TrackAlias: sharedAlias}) + }() + _, err := cli.Subscribe(t.Context(), &message.Subscribe{Namespace: ns, Name: []byte(name)}) + return err + } + // publish has srv PUBLISH name to cli with alias, and returns cli's + // AcceptPublish result. + publish := func(t *testing.T, cli, srv *session.Session, name string) error { + t.Helper() + go func() { + _, _ = srv.Publish(t.Context(), &message.Publish{Namespace: ns, Name: []byte(name), TrackAlias: alias}) + }() + r, err := cli.AcceptRequest(t.Context()) + if err != nil { + return err + } + _, err = r.AcceptPublish() + return err + } + + for _, tc := range []struct { + name string + first, second func(t *testing.T, cli, srv *session.Session, name string) error + }{ + {"SUBSCRIBE_OK after SUBSCRIBE_OK", subscribe, subscribe}, + {"PUBLISH after PUBLISH", publish, publish}, + {"PUBLISH after SUBSCRIBE_OK", subscribe, publish}, + {"SUBSCRIBE_OK after PUBLISH", publish, subscribe}, + } { + t.Run(tc.name, func(t *testing.T) { + cli, srv := openPair(t) + if err := tc.first(t, cli, srv, "trackA"); err != nil { + t.Fatalf("first: %v", err) + } + err := tc.second(t, cli, srv, "trackB") + if dupErr, ok := errors.AsType[*session.ErrDuplicateTrackAlias](err); !ok || dupErr.Alias != alias { + t.Fatalf("second: error = %v (%T), want *session.ErrDuplicateTrackAlias for %d", err, err, alias) + } + requireClosedCode(t, cli, moqt.SessionDuplicateTrackAlias) }) - wg.Go(func() { - stream, err := cli.Subscribe(ctx, &message.Subscribe{ - Namespace: ns, - Name: name, - }) + } +} + +// aliasHandle is the receiving side of a subscription: a [session.Subscription] +// or a [session.IncomingPublication]. +type aliasHandle interface { + io.Reader + Close() error + Broker() *session.RequestBroker +} + +// finishPublication has pub end with PUBLISH_DONE and FIN, and h consume them: +// through its broker when served, else by reading h itself. +func finishPublication(t *testing.T, h aliasHandle, pub *session.Publication, served bool) { + t.Helper() + done := make(chan error, 1) + go func() { done <- pub.Done(moqt.PublishDoneTrackEnded, "") }() + var err error + if served { + err = h.Broker().Serve(t.Context(), nil) + } else { + for err == nil { + _, err = message.Parse(h) + } + if errors.Is(err, io.EOF) { + err = nil + } + } + if err != nil { + t.Fatalf("consuming PUBLISH_DONE and FIN: %v", err) + } + if err := <-done; err != nil { + t.Fatalf("Done: %v", err) + } +} + +// establishOnAlias establishes a subscription to ns/name whose publisher, srv, +// assigns alias: cli's SUBSCRIBE answered with SUBSCRIBE_OK, or srv's PUBLISH +// accepted by cli. It returns both ends. +func establishOnAlias( + t *testing.T, cli, srv *session.Session, viaPublish bool, name string, alias uint64, +) (aliasHandle, *session.Publication) { + t.Helper() + ns := wire.TrackNamespace{[]byte("ns")} + pubs := make(chan *session.Publication, 1) + if viaPublish { + go func() { + p, err := srv.Publish(t.Context(), &message.Publish{Namespace: ns, Name: []byte(name), TrackAlias: alias}) if err != nil { - cliErr = err - return + t.Errorf("Publish: %v", err) } - _ = stream.Close() - }) - wg.Wait() - if srvErr != nil { - t.Fatalf("server: %v", srvErr) + pubs <- p + }() + r, err := cli.AcceptRequest(t.Context()) + if err != nil { + t.Fatalf("AcceptRequest: %v", err) + } + in, err := r.AcceptPublish() + if err != nil { + t.Fatalf("AcceptPublish: %v", err) + } + return in, <-pubs + } + go func() { + r, err := srv.AcceptRequest(t.Context()) + if err != nil { + t.Errorf("AcceptRequest: %v", err) + pubs <- nil + return + } + p, err := r.AcceptSubscribe(&message.SubscribeOK{TrackAlias: alias}) + if err != nil { + t.Errorf("AcceptSubscribe: %v", err) } - return cliErr + pubs <- p + }() + sub, err := cli.Subscribe(t.Context(), &message.Subscribe{Namespace: ns, Name: []byte(name)}) + if err != nil { + t.Fatalf("Subscribe: %v", err) } + return sub, <-pubs +} - // First subscribe: alias 7 → (ns, "trackA"). Should succeed. - if err := doSubscribe(wire.TrackNamespace{[]byte("ns")}, []byte("trackA")); err != nil { - t.Fatalf("first Subscribe: %v", err) +// TestAliasReleasedWhenTerminated: a Subscription or IncomingPublication +// releases its Track Alias once the subscription is Terminated (§5.1), so the +// publisher may then use it for a different Track (§11.1) without the session +// closing with DUPLICATE_TRACK_ALIAS. +func TestAliasReleasedWhenTerminated(t *testing.T) { + const alias = uint64(7) + for _, tc := range []struct { + name string + terminate func(t *testing.T, h aliasHandle, pub *session.Publication) + }{ + {"subscriber Close", func(_ *testing.T, h aliasHandle, _ *session.Publication) { _ = h.Close() }}, + {"subscriber Broker Close", func(_ *testing.T, h aliasHandle, _ *session.Publication) { + h.Broker().Close(moqt.StreamResetCancelled) + }}, + {"PUBLISH_DONE and FIN, served", func(t *testing.T, h aliasHandle, pub *session.Publication) { + finishPublication(t, h, pub, true) + }}, + {"PUBLISH_DONE and FIN, read", func(t *testing.T, h aliasHandle, pub *session.Publication) { + finishPublication(t, h, pub, false) + }}, + {"Serve cancelled", func(t *testing.T, h aliasHandle, _ *session.Publication) { + ctx, cancel := context.WithCancel(t.Context()) + cancel() + _ = h.Broker().Serve(ctx, nil) + }}, + } { + for _, viaPublish := range []bool{false, true} { + name := tc.name + ", SUBSCRIBE_OK" + if viaPublish { + name = tc.name + ", PUBLISH" + } + t.Run(name, func(t *testing.T) { + cli, srv := openPair(t) + h, pub := establishOnAlias(t, cli, srv, viaPublish, "trackA", alias) + tc.terminate(t, h, pub) + + establishOnAlias(t, cli, srv, false, "trackB", alias) + if err := cli.Err(); err != nil { + t.Fatalf("reusing a Terminated subscription's alias closed the session: %v", err) + } + }) + } } +} - // Second subscribe: alias 7 → (ns, "trackB"). Should fail with ErrDuplicateTrackAlias. - err := doSubscribe(wire.TrackNamespace{[]byte("ns")}, []byte("trackB")) - var dupErr *session.ErrDuplicateTrackAlias - if !errors.As(err, &dupErr) { - t.Fatalf("second Subscribe: error = %v (%T), want *session.ErrDuplicateTrackAlias", err, err) +// TestCloseAfterPublisherFinFINs: once the publisher has ended the +// subscription with PUBLISH_DONE and its FIN, Close FINs this side rather than +// resetting it (§3.3.2: the requester SHOULD FIN), whether the FIN was read +// through the handle or by its broker. +func TestCloseAfterPublisherFinFINs(t *testing.T) { + for _, served := range []bool{false, true} { + for _, viaPublish := range []bool{false, true} { + t.Run(fmt.Sprintf("served=%v publish=%v", served, viaPublish), func(t *testing.T) { + cli, srv := openPair(t) + h, pub := establishOnAlias(t, cli, srv, viaPublish, "trackA", 7) + finishPublication(t, h, pub, served) + if err := h.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + if _, err := message.Parse(pub); !errors.Is(err, io.EOF) { + t.Fatalf("publisher read after Close: %v, want io.EOF (a FIN)", err) + } + }) + } } - if dupErr.Alias != sharedAlias { - t.Errorf("Alias = %d, want %d", dupErr.Alias, sharedAlias) +} + +// TestAcceptPublishWriteFailureReleasesAlias: a PUBLISH whose REQUEST_OK +// cannot be written never becomes Established (§5.1), so its alias is free for +// a different Track (§11.1). +func TestAcceptPublishWriteFailureReleasesAlias(t *testing.T) { + const alias = uint64(7) + cli, srv := openPair(t) + streams := make(chan session.Stream, 1) + go func() { + stream, err := srv.OpenPublish(&message.Publish{ + Namespace: wire.TrackNamespace{[]byte("ns")}, Name: []byte("trackA"), TrackAlias: alias, + }) + if err != nil { + t.Errorf("OpenPublish: %v", err) + } + streams <- stream + }() + r, err := cli.AcceptRequest(t.Context()) + if err != nil { + t.Fatalf("AcceptRequest: %v", err) } + (<-streams).CancelRead(uint64(moqt.StreamResetCancelled)) + if _, err := r.AcceptPublish(); err == nil { + t.Fatal("AcceptPublish wrote REQUEST_OK to a stream the publisher stopped reading") + } + + establishOnAlias(t, cli, srv, false, "trackB", alias) + if err := cli.Err(); err != nil { + t.Fatalf("reusing the alias of a PUBLISH never accepted closed the session: %v", err) + } +} + +// TestAliasReleasedOncePerSubscription: two subscriptions to one Track share an +// alias (§5.1). Terminating one twice releases only its own registration, so +// the other keeps the alias Established and a different Track reusing it still +// closes the session (§11.1). +func TestAliasReleasedOncePerSubscription(t *testing.T) { + const alias = uint64(7) + cli, srv := openPair(t) + first, _ := establishOnAlias(t, cli, srv, false, "trackA", alias) + establishOnAlias(t, cli, srv, false, "trackA", alias) + + _ = first.Close() + first.Broker().Close(moqt.StreamResetCancelled) + _ = first.Broker().Serve(t.Context(), nil) + + go func() { + if r, err := srv.AcceptRequest(t.Context()); err == nil { + _ = r.Reply(&message.SubscribeOK{TrackAlias: alias}) + } + }() + _, _ = cli.Subscribe(t.Context(), &message.Subscribe{ + Namespace: wire.TrackNamespace{[]byte("ns")}, Name: []byte("trackB"), + }) + requireClosedCode(t, cli, moqt.SessionDuplicateTrackAlias) } // TestSubscribeSameTrackAliasShared verifies that subscribing to the same diff --git a/pkg/moqt/session/track_properties.go b/pkg/moqt/session/track_properties.go index 0ba9eab9..4fdea66e 100644 --- a/pkg/moqt/session/track_properties.go +++ b/pkg/moqt/session/track_properties.go @@ -77,22 +77,28 @@ func ValidateTrackProperties( return pairs, nil } -// CheckTrackProperties validates raw Track Properties against the types -// configured with [WithKnownMandatoryTrackProperties] (§2.5.1), returning +// CheckTrackProperties validates raw Track Properties: a value the draft makes +// session-fatal closes the session (§12.5, §12.6), and against the types +// configured with [WithKnownMandatoryTrackProperties] (§2.5.1) it returns // *ErrUnsupportedMandatoryTrackProperty or an error wrapping // [ErrMalformedTrackProperties]; see [TrackPropertiesRejectCode]. It is for // callers that bypass [Request.AcceptPublish] and the outbound openers, which -// already check. Without that option it returns nil. +// already check. Without that option only the values are checked. func (s *Session) CheckTrackProperties(raw []byte, context string) error { return s.validateTrackProperties(raw, context) } -// validateTrackProperties is a session-level convenience that uses the -// session's configured set of known mandatory track property types. -// -// If WithKnownMandatoryTrackProperties was never called (the map is nil), -// the check is skipped, for endpoints that pass Track Properties through. +// validateTrackProperties checks Track Properties received in context. A value +// the draft makes session-fatal (see [message.CheckTrackPropertyValues]) +// closes the session with PROTOCOL_VIOLATION. Then, against the session's +// configured set of known mandatory track property types, an unknown one is +// refused (§2.5.1); if WithKnownMandatoryTrackProperties was never called (the +// map is nil), that check is skipped, for endpoints that pass Track +// Properties through. func (s *Session) validateTrackProperties(raw []byte, context string) error { + if err := s.checkTrackPropertyValues(raw, context); err != nil { + return err + } if s.knownMandatoryTrackProperties == nil { return nil // not configured — skip enforcement } @@ -100,6 +106,25 @@ func (s *Session) validateTrackProperties(raw []byte, context string) error { return err } +// checkTrackPropertyValues closes the session with PROTOCOL_VIOLATION when +// Track Properties received in context hold a value the draft makes +// session-fatal (see [message.CheckTrackPropertyValues]): "If an endpoint +// receives a value outside this range, it MUST close the session" (§12.5, +// §12.6). Immutable Properties are searched when they parse (§12.7); Track +// Properties that do not parse are left to the caller. +func (s *Session) checkTrackPropertyValues(raw []byte, context string) error { + pairs, parseErr := message.ParseTrackProperties(raw) + if parseErr == nil { + if all, err := message.ExpandImmutable(pairs); err == nil { + pairs = all + } + } + if err := message.CheckTrackPropertyValues(pairs); err != nil { + return s.closeProtocolViolation(fmt.Errorf("%s: %w", context, err)) + } + return nil +} + // TrackPropertiesRejectCode is the REQUEST_ERROR code for a Track Properties // validation error: UNSUPPORTED_EXTENSION (§2.5.1) or MALFORMED_TRACK. func TrackPropertiesRejectCode(err error) moqt.RequestErrorCode { diff --git a/pkg/moqt/session/track_properties_test.go b/pkg/moqt/session/track_properties_test.go index 4c58f702..6321e724 100644 --- a/pkg/moqt/session/track_properties_test.go +++ b/pkg/moqt/session/track_properties_test.go @@ -778,3 +778,122 @@ func TestMandatoryTrackPropertyBoundaries(t *testing.T) { }) } } + +// TestTrackPropertyValueOutOfRangeCloses: a DEFAULT_PUBLISHER_GROUP_ORDER +// outside {1, 2} (§12.5) or a DYNAMIC_GROUPS above 1 (§12.6), also inside +// Immutable Properties (§12.7), closes the receiver with PROTOCOL_VIOLATION on +// each path Track Properties arrive by, whether or not it enforces Mandatory +// Track Properties. +func TestTrackPropertyValueOutOfRangeCloses(t *testing.T) { + t.Parallel() + prop := func(typ message.PropertyType, v uint64) []wire.KVPair { + return []wire.KVPair{{Type: typ, IntVal: v}} + } + values := []struct { + name string + props []byte + }{ + { + "DEFAULT_PUBLISHER_GROUP_ORDER 3", + message.AppendTrackProperties(prop(message.PropertyDefaultPublisherGroupOrder, 3)), + }, + { + "DEFAULT_PUBLISHER_GROUP_ORDER 0", + message.AppendTrackProperties(prop(message.PropertyDefaultPublisherGroupOrder, 0)), + }, + {"DYNAMIC_GROUPS 2", message.AppendTrackProperties(prop(message.PropertyDynamicGroups, 2))}, + {"inside Immutable Properties", message.AppendTrackProperties([]wire.KVPair{ + { + Type: message.PropertyImmutableProperties, + ByteVal: message.AppendTrackProperties(prop(message.PropertyDynamicGroups, 7)), + }, + })}, + {"next to Immutable Properties that do not parse", message.AppendTrackProperties([]wire.KVPair{ + {Type: message.PropertyDynamicGroups, IntVal: 2}, + {Type: message.PropertyImmutableProperties, ByteVal: []byte{0x40}}, + })}, + } + ns := wire.TrackNamespace{[]byte("ns")} + paths := []struct { + name string + // run makes the receiver get props, returning it. + run func(t *testing.T, cli, srv *session.Session, props []byte) *session.Session + }{ + {"SUBSCRIBE_OK", func(t *testing.T, cli, srv *session.Session, props []byte) *session.Session { + go func() { + if r, err := srv.AcceptRequest(t.Context()); err == nil { + _ = r.Reply(&message.SubscribeOK{TrackAlias: 7, TrackProperties: props}) + } + }() + go func() { _, _ = cli.Subscribe(t.Context(), &message.Subscribe{Namespace: ns, Name: []byte("t")}) }() + return cli + }}, + {"FETCH_OK", func(t *testing.T, cli, srv *session.Session, props []byte) *session.Session { + go func() { + if r, err := srv.AcceptRequest(t.Context()); err == nil { + _ = r.Reply(&message.FetchOK{EndLocation: message.Location{Group: 1}, TrackProperties: props}) + } + }() + go func() { _, _ = cli.Fetch(t.Context(), &message.Fetch{Namespace: ns, Name: []byte("t")}) }() + return cli + }}, + {"TRACK_STATUS_OK", func(t *testing.T, cli, srv *session.Session, props []byte) *session.Session { + go func() { + if r, err := srv.AcceptRequest(t.Context()); err == nil { + _ = r.Reply(&message.RequestOK{TrackProperties: props}) + } + }() + go func() { _, _ = cli.TrackStatus(t.Context(), &message.TrackStatus{Namespace: ns, Name: []byte("t")}) }() + return cli + }}, + {"SUBSCRIBE_TRACKS_OK", func(t *testing.T, cli, srv *session.Session, props []byte) *session.Session { + go func() { + if r, err := srv.AcceptRequest(t.Context()); err == nil { + _ = r.Reply(&message.RequestOK{TrackProperties: props}) + } + }() + go func() { + _, _ = cli.SubscribeTracks(t.Context(), &message.SubscribeTracks{TrackNamespacePrefix: ns}) + }() + return cli + }}, + { + "PUBLISH, rejected by the application", + func(t *testing.T, cli, srv *session.Session, props []byte) *session.Session { + go func() { + _, _ = cli.Publish(t.Context(), &message.Publish{ + Namespace: ns, Name: []byte("t"), TrackAlias: 1, TrackProperties: props, + }) + }() + go func() { + if r, err := srv.AcceptRequest(t.Context()); err == nil { + _ = r.RejectError(moqt.RequestUnauthorized, "no") + } + }() + return srv + }, + }, + {"PUBLISH", func(t *testing.T, cli, srv *session.Session, props []byte) *session.Session { + go func() { + _, _ = cli.Publish(t.Context(), &message.Publish{ + Namespace: ns, Name: []byte("t"), TrackAlias: 1, TrackProperties: props, + }) + }() + go func() { + if r, err := srv.AcceptRequest(t.Context()); err == nil { + _, _ = r.AcceptPublish() + } + }() + return srv + }}, + } + for _, p := range paths { + for _, v := range values { + t.Run(p.name+"/"+v.name, func(t *testing.T) { + t.Parallel() + cli, srv := openPair(t) + requireClosedProtocolViolation(t, p.run(t, cli, srv, v.props)) + }) + } + } +} diff --git a/pkg/moqt/session/trackalias.go b/pkg/moqt/session/trackalias.go index d515ba9f..5008726d 100644 --- a/pkg/moqt/session/trackalias.go +++ b/pkg/moqt/session/trackalias.go @@ -5,6 +5,7 @@ import ( "fmt" "time" + "github.com/floatdrop/moq-go/pkg/moqt" "github.com/floatdrop/moq-go/pkg/moqt/message" "github.com/floatdrop/moq-go/pkg/moqt/track" ) @@ -22,7 +23,8 @@ func (s *Session) AllocOutboundTrackAlias() uint64 { // ErrDuplicateTrackAlias is returned by [Session.RegisterInboundTrack] when the // peer assigns a Track Alias that is already in use for a different track -// (§11.1). The caller MUST close the session with SessionDuplicateTrackAlias. +// (§11.1). The session is already closed with +// [moqt.SessionDuplicateTrackAlias]. type ErrDuplicateTrackAlias struct { Alias uint64 Existing track.Key @@ -64,20 +66,28 @@ type InboundTrack struct { // a shared alias carries, and a release cannot tell which registration it // ends, so the survivor may keep a released one's properties. // -// If alias is registered for a different track, *ErrDuplicateTrackAlias is -// returned and the caller MUST close the session with -// SessionDuplicateTrackAlias (§11.1). +// If alias is registered for a different track, the session is closed with +// DUPLICATE_TRACK_ALIAS and *ErrDuplicateTrackAlias returned: "it MUST close +// the session with error DUPLICATE_TRACK_ALIAS" (§11.1). func (s *Session) RegisterInboundTrack(alias uint64, key track.Key, trackProperties []byte) error { in := InboundTrack{ Key: key, DefaultPublisherPriority: message.TrackDefaultPublisherPriority(trackProperties), } in.MaxCacheDuration, in.HasMaxCacheDuration = message.TrackMaxCacheDuration(trackProperties) + if err := s.registerInboundTrack(alias, in); err != nil { + _ = s.Close(moqt.SessionDuplicateTrackAlias, err.Error()) + return err + } + return nil +} + +func (s *Session) registerInboundTrack(alias uint64, in InboundTrack) error { s.mu.Lock() defer s.mu.Unlock() if existing, ok := s.inboundAliases[alias]; ok { - if existing.Key != key { - return &ErrDuplicateTrackAlias{Alias: alias, Existing: existing.Key, New: key} + if existing.Key != in.Key { + return &ErrDuplicateTrackAlias{Alias: alias, Existing: existing.Key, New: in.Key} } s.inboundAliases[alias] = in s.inboundAliasRefs[alias]++ @@ -98,7 +108,9 @@ func (s *Session) RegisterInboundTrackAlias(alias uint64, key track.Key) error { // UnregisterInboundTrackAlias releases one registration of alias (see // [Session.RegisterInboundTrack]); the last release removes it. Call it once -// the subscription or publication is torn down, after a grace period (§11.1). +// the subscription is Terminated (§5.1), for an alias registered with +// RegisterInboundTrack directly: a [Subscription] or [IncomingPublication] +// releases its own. // // Unregistering an alias that was never registered is a no-op. func (s *Session) UnregisterInboundTrackAlias(alias uint64) { diff --git a/pkg/relay/handler_fetch.go b/pkg/relay/handler_fetch.go index 94b99403..a507d448 100644 --- a/pkg/relay/handler_fetch.go +++ b/pkg/relay/handler_fetch.go @@ -80,12 +80,9 @@ func (h *sessionHandler) handleFetch(ctx context.Context, req *session.Request, // draft-20 moved the FETCH range out of the message and into the // LOCATION_FILTER parameter (§5.1.2), inclusive at both ends. An absent - // filter fetches the whole track up to Largest Object. - filter, err := message.LocationFilterFromParam(msg.Parameters) - if err != nil { - _ = req.RejectError(moqt.RequestInvalidFilter, "relay: malformed LOCATION_FILTER") - return - } + // filter fetches the whole track up to Largest Object. AcceptRequest has + // validated it (see [message.Parameters.CheckScope]). + filter, _ := message.LocationFilterFromParam(msg.Parameters) if filter == nil { filter = &message.LocationFilter{} } @@ -218,22 +215,11 @@ func (h *sessionHandler) handleFetchUpdate(ctx context.Context, req *session.Req } } -// TODO: §10.2.8: an out-of-range GROUP_ORDER MUST close the session, as -// [checkGroupOrderParam] does for SUBSCRIBE; the FETCH path reads it as -// Ascending. -// -// fetchGroupOrder pulls the GROUP_ORDER parameter (§10.2.8) out of a -// FETCH's Parameters list. Defaults to ascending when omitted; the -// FETCH responder uses this to choose between ascending and descending -// traversal through the cache. +// fetchGroupOrder is a FETCH's GROUP_ORDER (§10.2.8): Ascending when omitted. +// AcceptRequest has closed the session on a value outside {1, 2}. func fetchGroupOrder(ps message.Parameters) message.GroupOrder { - p, ok := ps.Find(message.ParamGroupOrder) - if !ok { - return message.GroupOrderAscending - } - g := message.GroupOrder(p.Byte) - if g == message.GroupOrderDescending { - return g + if p, ok := ps.Find(message.ParamGroupOrder); ok { + return message.GroupOrder(p.Byte) } return message.GroupOrderAscending } diff --git a/pkg/relay/handler_fill.go b/pkg/relay/handler_fill.go index ddd002fb..d4a45df6 100644 --- a/pkg/relay/handler_fill.go +++ b/pkg/relay/handler_fill.go @@ -20,9 +20,9 @@ import ( // requestID is the Request ID of the message that asked for the fill; the // FETCH_HEADER carries it, so one subscription can have several fills open. // -// It returns an error only for a malformed FILL_PARAMETERS, which the caller -// MUST turn into a session-level PROTOCOL_VIOLATION (§10.2.15). Any other -// failure resets the fill stream and leaves the subscription unaffected. +// A failure resets the fill stream, leaves the subscription unaffected, and +// is returned for the log. AcceptRequest has closed the session on a malformed +// FILL_PARAMETERS (§10.2.15; see [message.Parameters.CheckScope]). func (h *sessionHandler) maybeServeFill( ctx context.Context, sub *registry.DownstreamSub, @@ -31,10 +31,7 @@ func (h *sessionHandler) maybeServeFill( requestID uint64, ps message.Parameters, ) error { - inner, requested, err := message.FillParametersFromParam(ps) - if err != nil { - return err - } + inner, requested, _ := message.FillParametersFromParam(ps) if !requested { return nil } @@ -62,10 +59,7 @@ func (h *sessionHandler) maybeServeFill( // §5.1.3: the fill range comes from the LOCATION_FILTER inside // FILL_PARAMETERS, falling back to the subscription's own filter, and to // the whole track when neither is present. - filter, err := message.LocationFilterFromParam(inner) - if err != nil { - return fail(err) - } + filter, _ := message.LocationFilterFromParam(inner) if filter == nil { filter = sub.GetFilter() } @@ -94,6 +88,7 @@ func (h *sessionHandler) maybeServeFill( // would (§5.1.4); the other types are inherited. rangeFilters := sub.GetRangeFilters() if slices.ContainsFunc(inner, func(p message.Parameter) bool { return message.IsRangeFilterParam(p.Type) }) { + var err error rangeFilters, err = rangeFilters.Update(inner) if err == nil && rangeFilters != nil { err = rangeFilters.Validate(h.sess.MaxFilterRanges()) diff --git a/pkg/relay/handler_forward.go b/pkg/relay/handler_forward.go index 7abb9e4c..d015059f 100644 --- a/pkg/relay/handler_forward.go +++ b/pkg/relay/handler_forward.go @@ -127,5 +127,5 @@ func (h *sessionHandler) serveForwardedPublish( if p, ok := params.Find(message.ParamNewGroupRequest); ok { h.propagateNewGroupUpstream(ctx, fullName, p.Varint) } - h.readSubscribeUpdates(ctx, stream, sub, fullName) + h.readSubscribeUpdates(ctx, stream, sub, fullName, true) } diff --git a/pkg/relay/handler_namespace.go b/pkg/relay/handler_namespace.go index ee829f62..6dd4ad00 100644 --- a/pkg/relay/handler_namespace.go +++ b/pkg/relay/handler_namespace.go @@ -270,15 +270,8 @@ func (h *sessionHandler) handleSubscribeTracks( // subscribeTracksForwarding resolves a SUBSCRIBE_TRACKS's FORWARD (§10.2.18, // default true) and GROUP_ORDER (§10.2.8, 0 when omitted), which §10.20.1 -// copies onto forwarded PUBLISHes. An out-of-range value is a -// *paramProtocolViolation. -func subscribeTracksForwarding(ps message.Parameters) (forward bool, groupOrder byte, err error) { - if err := checkForwardParam(ps); err != nil { - return false, 0, err - } - if err := checkGroupOrderParam(ps); err != nil { - return false, 0, err - } +// copies onto forwarded PUBLISHes. +func subscribeTracksForwarding(ps message.Parameters) (forward bool, groupOrder byte) { forward = true if p, ok := ps.Find(message.ParamForward); ok { forward = p.Byte != 0 @@ -286,7 +279,7 @@ func subscribeTracksForwarding(ps message.Parameters) (forward bool, groupOrder if p, ok := ps.Find(message.ParamGroupOrder); ok { groupOrder = p.Byte } - return forward, groupOrder, nil + return forward, groupOrder } // serveNamespaceFollowups holds a namespace request stream open and answers @@ -449,15 +442,7 @@ func (h *sessionHandler) tracksUpdate( tokens = updTokens params, err := h.resolveTracksParams(merged) if err != nil { - if _, ok := errors.AsType[*paramProtocolViolation](err); ok { - _ = h.sess.Close(moqt.SessionProtocolViolation, err.Error()) - return false - } - code := moqt.RequestMalformedTrack - if errors.Is(err, message.ErrInvalidFilter) { - code = moqt.RequestInvalidFilter - } - e.Finish(&message.RequestError{ErrorCode: code, ErrorReason: err.Error()}) + e.Finish(&message.RequestError{ErrorCode: moqt.RequestInvalidFilter, ErrorReason: err.Error()}) return endAfterFinish(ctx, e) } oldPrefix := *cur @@ -571,15 +556,11 @@ func mergeTracksUpdate(stored, upd message.Parameters) message.Parameters { return out } -// resolveTracksParams validates a SUBSCRIBE_TRACKS's parameters on a -// SUBSCRIBE's terms (§10.20.1; errors as for -// [sessionHandler.refuseSubscriptionParams]) and MAX_FILTER_RANGES (§5.1.4), -// and resolves what forwarding needs. +// resolveTracksParams validates a SUBSCRIBE_TRACKS's Range Filters on a +// SUBSCRIBE's terms (§10.20.1) and MAX_FILTER_RANGES (§5.1.4), and resolves +// what forwarding needs. An error is INVALID_FILTER. func (h *sessionHandler) resolveTracksParams(ps message.Parameters) (*registry.TracksParams, error) { - forward, groupOrder, err := subscribeTracksForwarding(ps) - if err != nil { - return nil, err - } + forward, groupOrder := subscribeTracksForwarding(ps) rangeFilters, err := message.RangeFiltersFromParams(ps) if err == nil && rangeFilters != nil { err = rangeFilters.Validate(h.sess.MaxFilterRanges()) diff --git a/pkg/relay/handler_publish.go b/pkg/relay/handler_publish.go index 34d8c57e..cd26f353 100644 --- a/pkg/relay/handler_publish.go +++ b/pkg/relay/handler_publish.go @@ -7,7 +7,6 @@ import ( "slices" "sync/atomic" - "github.com/floatdrop/moq-go/pkg/moqt" "github.com/floatdrop/moq-go/pkg/moqt/message" "github.com/floatdrop/moq-go/pkg/moqt/session" "github.com/floatdrop/moq-go/pkg/moqt/track" @@ -39,14 +38,10 @@ func (h *sessionHandler) handlePublish(ctx context.Context, req *session.Request slog.String("name", string(msg.Name)), slog.Uint64("alias", msg.TrackAlias)) - // §10.2.18: an out-of-range FORWARD closes the session. - if err := checkForwardParam(msg.Parameters); err != nil { - _ = h.sess.Close(moqt.SessionProtocolViolation, err.Error()) - return - } - // §2.5.1: refuse an unknown Mandatory Track Property. MALFORMED_TRACK for // unparseable Track Properties is this repo's choice; the draft is silent. + // AcceptRequest has closed the session on a session-fatal value (§12.5, + // §12.6). if err := h.sess.CheckTrackProperties(msg.TrackProperties, "PUBLISH"); err != nil { _ = req.RejectError(session.TrackPropertiesRejectCode(err), err.Error()) return @@ -65,23 +60,25 @@ func (h *sessionHandler) handlePublish(ctx context.Context, req *session.Request _, createdEntry := h.tracks.GetOrCreateNew(fullName) // §11.1: register the publisher's chosen alias so the fanout path can map - // it back to the track and duplicates are detected. A duplicate alias is a - // session-level error per spec, but we scope the failure to this request. + // it back to the track. A duplicate closed the session. if err := h.sess.RegisterInboundTrack(msg.TrackAlias, fullName.Key(), msg.TrackProperties); err != nil { h.log.LogAttrs(ctx, slog.LevelDebug, "PUBLISH alias registration failed", slog.String("err", err.Error())) if createdEntry { h.tracks.DeleteIfUnused(fullName) } - _ = req.RejectError(moqt.RequestMalformedTrack, err.Error()) return } if hook := testHookAfterAliasRegistered.Load(); hook != nil { (*hook)(fullName) } - // The publisher sent the PUBLISH, so it may send REQUEST_UPDATE (§10.9). - sub := registry.NewUpstreamSub(h.allocSubID(), h.sess, req.Stream, msg.TrackAlias, msg.RequestID, true) + // The publisher sent the PUBLISH, so it may send REQUEST_UPDATE (§10.9), + // in the scope an accepted PUBLISH's publisher has. + broker := h.sess.NewRequestBroker(req.Stream) + broker.PeerMessages(true, true) + broker.UpdateScope(message.ScopeUpdateFromPublisher) + sub := registry.NewUpstreamSub(h.allocSubID(), h.sess, req.Stream, broker, msg.TrackAlias, msg.RequestID) // §5.1: the PUBLISH sets the initial Forward State (default 1). if f, ok := msg.Parameters.Find(message.ParamForward); ok && f.Byte == 0 { sub.SetForwardState(0) diff --git a/pkg/relay/handler_publish_alias_window_test.go b/pkg/relay/handler_publish_alias_window_test.go index 40f56a1e..b9e183ce 100644 --- a/pkg/relay/handler_publish_alias_window_test.go +++ b/pkg/relay/handler_publish_alias_window_test.go @@ -81,9 +81,10 @@ func TestPublish_TrackEntryPrecedesAliasRouting(t *testing.T) { waitRelayLargest(t, probe, video, name, groupID, 0) } -// TestPublish_RejectedAliasLeavesTrackUnknown: a rejected PUBLISH leaves no -// track entry behind, so a FETCH is DOES_NOT_EXIST (§10.6), not INVALID_RANGE. -func TestPublish_RejectedAliasLeavesTrackUnknown(t *testing.T) { +// TestPublish_DuplicateAliasLeavesTrackUnknown: a PUBLISH whose duplicate +// alias closes the session leaves no track entry behind, so a FETCH is +// DOES_NOT_EXIST (§10.6), not INVALID_RANGE. +func TestPublish_DuplicateAliasLeavesTrackUnknown(t *testing.T) { video := ns("video") const alias = uint64(91) first := []byte("cam-alias-taken") @@ -100,12 +101,12 @@ func TestPublish_RejectedAliasLeavesTrackUnknown(t *testing.T) { } t.Cleanup(func() { _ = pub.Close() }) - // §11.1: the alias is taken, so this PUBLISH is rejected — after the - // relay has already created the entry for `second`. + // §11.1: the alias is taken, so this PUBLISH closes the session — after + // the relay has already created the entry for `second`. if _, err := pubSess.Publish(t.Context(), &message.Publish{ Namespace: video, Name: second, TrackAlias: alias, }); err == nil { - t.Fatal("duplicate Track Alias PUBLISH was accepted, want rejection") + t.Fatal("duplicate Track Alias PUBLISH was accepted, want the session closed") } fetcher := dialAnotherClient(t, pubSess) diff --git a/pkg/relay/handler_subscribe.go b/pkg/relay/handler_subscribe.go index 30cbfd72..bb9560ec 100644 --- a/pkg/relay/handler_subscribe.go +++ b/pkg/relay/handler_subscribe.go @@ -160,8 +160,8 @@ func (h *sessionHandler) handleSubscribe(ctx context.Context, req *session.Reque slog.String("name", string(msg.Name)), slog.Uint64("alias", alias)) - // §5.1.3: FILL_PARAMETERS asks for a fill fetch stream; a malformed one - // was already rejected by installSubscribeParams. + // §5.1.3: FILL_PARAMETERS asks for a fill fetch stream; AcceptRequest + // has closed the session on a malformed one. if err := h.maybeServeFill(ctx, sub, entry, fullName, msg.RequestID, msg.Parameters); err != nil { h.log.LogAttrs(ctx, slog.LevelDebug, "fill fetch stream not opened", slog.String("err", err.Error())) @@ -176,7 +176,7 @@ func (h *sessionHandler) handleSubscribe(ctx context.Context, req *session.Reque h.propagateForwardUpstream(ctx, fullName) } - h.readSubscribeUpdates(ctx, req.Stream, sub, fullName) + h.readSubscribeUpdates(ctx, req.Stream, sub, fullName, false) h.log.LogAttrs(ctx, slog.LevelDebug, "SUBSCRIBE stream ended", slog.String("name", string(msg.Name))) } @@ -185,18 +185,31 @@ func (h *sessionHandler) handleSubscribe(ctx context.Context, req *session.Reque // SUBSCRIBE's stream to [sessionHandler.handleSubscribeUpdate] until the // subscriber cancels, the stream turns undecodable (see [readRequestStream]) // or ctx ends. A subscriber FIN is not a cancellation (§3.3.2): the -// subscription lives on in [awaitRequestEnd]. +// subscription lives on in [awaitRequestEnd]. forwarded marks a PUBLISH the +// relay forwarded, which the subscriber answered. func (h *sessionHandler) readSubscribeUpdates( ctx context.Context, stream session.Stream, sub *registry.DownstreamSub, fullName track.FullTrackName, + forwarded bool, ) { updates := h.sess.NewRequestUpdateLimiter() fin := readRequestStream(ctx, h.sess, stream, func(m message.Message) bool { if h.isPeerStateNotify(m) { return false } + switch m.(type) { + case *message.RequestOK, *message.RequestError: + // On a forwarded PUBLISH the subscriber answered already, and + // the relay sends no REQUEST_UPDATE here: a second PUBLISH_OK + // or REQUEST_ERROR (§5.1). + if forwarded { + _ = h.sess.Close(moqt.SessionProtocolViolation, + fmt.Sprintf("%s after the PUBLISH_OK", m.Type())) + return false + } + } if upd, ok := m.(*message.RequestUpdate); ok { // §10.2.1: parameters outside the scope of a subscriber's // update are session-fatal. @@ -238,23 +251,12 @@ func (h *sessionHandler) handleSubscribeUpdate( ) { prevForward := sub.ForwardState() if err := installSubscribeParams(sub, upd.Parameters); err != nil { - if _, ok := errors.AsType[*paramProtocolViolation](err); ok { - // §10.2.8 / §10.2.18: session-level even in a REQUEST_UPDATE. - h.log.LogAttrs(ctx, slog.LevelDebug, "REQUEST_UPDATE parameter protocol violation", - slog.String("err", err.Error())) - _ = h.sess.Close(moqt.SessionProtocolViolation, err.Error()) - return - } - h.log.LogAttrs(ctx, slog.LevelDebug, "REQUEST_UPDATE parameter parse failed", + h.log.LogAttrs(ctx, slog.LevelDebug, "REQUEST_UPDATE range filter rejected", slog.String("err", err.Error())) // §10.9.1: REQUEST_ERROR, then PUBLISH_DONE / UPDATE_FAILED. Writes go // through the sub's lock. - code := moqt.RequestMalformedTrack - if errors.Is(err, message.ErrInvalidFilter) { - code = moqt.RequestInvalidFilter - } _ = sub.WriteMessage(&message.RequestError{ - ErrorCode: code, + ErrorCode: moqt.RequestInvalidFilter, ErrorReason: err.Error(), }) sub.TerminateWithPublishDone(moqt.PublishDoneUpdateFailed, err.Error()) @@ -309,8 +311,8 @@ func (h *sessionHandler) propagateNewGroupUpstream( dynamic, err := entry.DynamicGroups() if err != nil { - // §12.6: a bad DYNAMIC_GROUPS only declines the request here. - h.log.LogAttrs(ctx, slog.LevelDebug, "NEW_GROUP_REQUEST: bad DYNAMIC_GROUPS property", + // Unparseable Track Properties only decline the request here. + h.log.LogAttrs(ctx, slog.LevelDebug, "NEW_GROUP_REQUEST: unparseable Track Properties", slog.String("err", err.Error())) return } @@ -486,10 +488,10 @@ func (h *sessionHandler) subscribeUpstreamOnSession( (*hook)(fullName) } - // The relay is the requester, so the publisher may not send - // REQUEST_UPDATE (§10.9). - upstreamSub := registry.NewUpstreamSub( - h.allocSubID(), sess, upstreamStream, upstreamStream.OK.TrackAlias, subMsg.RequestID, false) + // The Subscription's broker: the publisher may not send REQUEST_UPDATE + // (§10.9), and the session releases the alias when it ends (§11.1). + upstreamSub := registry.NewUpstreamSub(h.allocSubID(), sess, upstreamStream, upstreamStream.Broker(), + upstreamStream.OK.TrackAlias, subMsg.RequestID) upstreamSub.SetFilter(filter) // Match the Forward=0 sent upstream: NewUpstreamSub starts at 1, and a // later §9.2 resume skips upstreams already at 1. @@ -509,8 +511,6 @@ func (h *sessionHandler) subscribeUpstreamOnSession( h.relayGo(func() { h.serveUpstreamStream(upstreamStream.Context(), upstreamSub) h.tracks.RemoveUpstream(fullName, upstreamSub.ID) - // Drop the alias with the subscription, so a peer may reuse it (§11.1). - sess.UnregisterInboundTrackAlias(upstreamStream.OK.TrackAlias) }) return entry, upstreamSub, nil @@ -576,22 +576,14 @@ func resolveGroupOrder(sub *registry.DownstreamSub, entry *registry.TrackEntry) // installSubscribeParams records the subscription parameters present in ps // (§10.2) on sub, leaving absent ones unchanged. The Largest snapshot is the -// caller's (see [registry.TrackRegistry.AddDownstreamSnapshotLargest]). +// caller's (see [registry.TrackRegistry.AddDownstreamSnapshotLargest]). The +// session has already closed on a value the draft makes session-fatal (see +// [message.Parameters.CheckScope]); an error here is a Range Filter's, which +// is INVALID_FILTER (§5.1.4). func installSubscribeParams(sub *registry.DownstreamSub, ps message.Parameters) error { - filter, err := message.LocationFilterFromParam(ps) - if err != nil { - return fmt.Errorf("location filter: %w", err) - } - if filter != nil { - if err := filter.Validate(); err != nil { - return err - } + if filter, _ := message.LocationFilterFromParam(ps); filter != nil { sub.SetFilter(filter) } - - if err := checkForwardParam(ps); err != nil { - return err - } if p, ok := ps.Find(message.ParamForward); ok { sub.SetForwardState(int(p.Byte)) } @@ -599,9 +591,6 @@ func installSubscribeParams(sub *registry.DownstreamSub, ps message.Parameters) if p, ok := ps.Find(message.ParamSubscriberPriority); ok { sub.SetPriority(p.Byte) } - if err := checkGroupOrderParam(ps); err != nil { - return err - } if p, ok := ps.Find(message.ParamGroupOrder); ok { sub.SetGroupOrder(p.Byte) } @@ -634,61 +623,18 @@ func installSubscribeParams(sub *registry.DownstreamSub, ps message.Parameters) } // refuseSubscriptionParams answers a SUBSCRIBE or SUBSCRIBE_TRACKS whose -// subscription parameters [installSubscribeParams] rejected. +// Range Filters [installSubscribeParams] rejected: malformed or over the limit, +// INVALID_FILTER (§5.1.4, §10.6). func (h *sessionHandler) refuseSubscriptionParams(ctx context.Context, req *session.Request, err error) { - if _, ok := errors.AsType[*paramProtocolViolation](err); ok { - // §10.2.8 / §10.2.18: an out-of-range GROUP_ORDER/FORWARD is a - // session-level PROTOCOL_VIOLATION. - h.log.LogAttrs(ctx, slog.LevelDebug, "subscription parameter protocol violation", - slog.String("err", err.Error())) - _ = h.sess.Close(moqt.SessionProtocolViolation, err.Error()) - return - } - // §5.1.4 / §10.6: a malformed or over-limit Range Filter is INVALID_FILTER. - if errors.Is(err, message.ErrInvalidFilter) { - h.log.LogAttrs(ctx, slog.LevelDebug, "subscription range filter rejected", - slog.String("err", err.Error())) - _ = req.RejectError(moqt.RequestInvalidFilter, err.Error()) - return - } - // Deviation: §5.1.2 makes a malformed LOCATION_FILTER a session-level - // PROTOCOL_VIOLATION; the relay scopes it to the request. - h.log.LogAttrs(ctx, slog.LevelDebug, "subscription parameter parse failed", + h.log.LogAttrs(ctx, slog.LevelDebug, "subscription range filter rejected", slog.String("err", err.Error())) - _ = req.RejectError(moqt.RequestMalformedTrack, err.Error()) + _ = req.RejectError(moqt.RequestInvalidFilter, err.Error()) } // errPeerGoingAway reports a request the relay did not send because the peer // sent GOAWAY (§10.4; see [peerSentGoaway]). var errPeerGoingAway = errors.New("relay: peer sent GOAWAY; no new requests to it (§10.4)") -// paramProtocolViolation marks a parameter value that closes the session with -// PROTOCOL_VIOLATION: an out-of-range GROUP_ORDER (§10.2.8) or FORWARD -// (§10.2.18). -type paramProtocolViolation struct{ reason string } - -func (e *paramProtocolViolation) Error() string { return e.reason } - -// checkForwardParam enforces the §10.2.18 FORWARD range (0 or 1). -func checkForwardParam(ps message.Parameters) error { - if p, ok := ps.Find(message.ParamForward); ok && p.Byte > 1 { - return ¶mProtocolViolation{fmt.Sprintf("invalid FORWARD value 0x%X (§10.2.18)", p.Byte)} - } - return nil -} - -// checkGroupOrderParam enforces the §10.2.8 GROUP_ORDER range (0x1 or 0x2). -func checkGroupOrderParam(ps message.Parameters) error { - if p, ok := ps.Find(message.ParamGroupOrder); ok { - switch message.GroupOrder(p.Byte) { - case message.GroupOrderAscending, message.GroupOrderDescending: - default: - return ¶mProtocolViolation{fmt.Sprintf("invalid GROUP_ORDER value 0x%X (§10.2.8)", p.Byte)} - } - } - return nil -} - // includeProperties reports whether INCLUDE_PROPERTIES (§10.2.21) asks for // Track Properties: yes unless it is 0. func includeProperties(ps message.Parameters) bool { diff --git a/pkg/relay/helpers_test.go b/pkg/relay/helpers_test.go index c54fe6fc..0e6f6e97 100644 --- a/pkg/relay/helpers_test.go +++ b/pkg/relay/helpers_test.go @@ -32,6 +32,14 @@ func trackProp(typ, v uint64) []wire.KVPair { return []wire.KVPair{{Type: typ, IntVal: v}} } +// badRangeFilter is a PRIORITY_FILTER ranging past 255, which the relay +// refuses per request with INVALID_FILTER (§10.2.12). +func badRangeFilter() message.Parameter { + return message.RangeFilterParam(&message.RangeFilter{ + Type: message.ParamPriorityFilter, Ranges: []message.Range{{Start: 0, End: 300}}, + }) +} + // dynamicGroupsProperties is Track Properties carrying DYNAMIC_GROUPS = value. func dynamicGroupsProperties(value uint64) []byte { return message.AppendTrackProperties(trackProp(message.PropertyDynamicGroups, value)) diff --git a/pkg/relay/internal/registry/broker_test.go b/pkg/relay/internal/registry/broker_test.go index 0d7611c6..bb56cfce 100644 --- a/pkg/relay/internal/registry/broker_test.go +++ b/pkg/relay/internal/registry/broker_test.go @@ -14,7 +14,8 @@ import ( // with [session.ErrRequestStreamClosed]. func TestUpstreamSub_UpdateDelegatesToBroker(t *testing.T) { t.Parallel() - sub := registry.NewUpstreamSub(1, nil, stubStream{}, 0, 7, false) + var sess *session.Session + sub := registry.NewUpstreamSub(1, nil, stubStream{}, sess.NewRequestBroker(stubStream{}), 0, 7) sub.CloseOnDemand() if !sub.IsTerminated() { diff --git a/pkg/relay/internal/registry/dynamic_groups_test.go b/pkg/relay/internal/registry/dynamic_groups_test.go index eea72b96..a05de61a 100644 --- a/pkg/relay/internal/registry/dynamic_groups_test.go +++ b/pkg/relay/internal/registry/dynamic_groups_test.go @@ -19,8 +19,8 @@ func dynamicGroupsProps(t *testing.T, value uint64) []byte { // TestTrackEntry_DynamicGroups pins the §12.6 DYNAMIC_GROUPS decode that // SetProperties performs once and caches: an absent property and value 0 are -// false, value 1 is true, and value > 1 is a PROTOCOL_VIOLATION surfaced as an -// error. +// false, and value 1 is true. The session closes on a value above 1 (§12.6) +// before Properties reach the registry. func TestTrackEntry_DynamicGroups(t *testing.T) { t.Parallel() @@ -51,12 +51,6 @@ func TestTrackEntry_DynamicGroups(t *testing.T) { t.Fatalf("got (%v, %v), want (true, nil)", got, err) } }) - t.Run("value > 1 is an error (§12.6)", func(t *testing.T) { - t.Parallel() - if _, err := setProps(dynamicGroupsProps(t, 2)).DynamicGroups(); err == nil { - t.Fatal("got nil error, want §12.6 protocol-violation error") - } - }) t.Run("malformed block is an error", func(t *testing.T) { t.Parallel() // 0x40 is the first byte of a 2-byte varint with no second byte, so @@ -92,6 +86,10 @@ func TestTrackEntry_DefaultGroupOrder(t *testing.T) { {"inside Immutable Properties", message.AppendTrackProperties([]wire.KVPair{ {Type: message.PropertyImmutableProperties, ByteVal: order(2)}, }), message.GroupOrderDescending}, + {"mutable Ascending over immutable Descending", message.AppendTrackProperties([]wire.KVPair{ + {Type: message.PropertyDefaultPublisherGroupOrder, IntVal: 1}, + {Type: message.PropertyImmutableProperties, ByteVal: order(2)}, + }), message.GroupOrderAscending}, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() diff --git a/pkg/relay/internal/registry/properties.go b/pkg/relay/internal/registry/properties.go index 48b61c89..a61a29c6 100644 --- a/pkg/relay/internal/registry/properties.go +++ b/pkg/relay/internal/registry/properties.go @@ -1,7 +1,6 @@ package registry import ( - "fmt" "slices" "github.com/floatdrop/moq-go/pkg/moqt/message" @@ -23,11 +22,9 @@ type decodedProperties struct { // set, no field below is meaningful, so every accessor reports it. parseErr error - // dynamicGroups is DYNAMIC_GROUPS=1 (§12.6). dynamicGroupsErr is a §12.6 - // PROTOCOL_VIOLATION (a DYNAMIC_GROUPS value > 1) by the upstream - // publisher. - dynamicGroups bool - dynamicGroupsErr error + // dynamicGroups is DYNAMIC_GROUPS=1 (§12.6). The session closed on a + // value above 1. + dynamicGroups bool // deliveryTimeouts is the publisher's Track-level OBJECT_DELIVERY_TIMEOUT // (§12.2) and SUBGROUP_DELIVERY_TIMEOUT (§12.1) pair. Per §8 a zero value @@ -36,15 +33,15 @@ type decodedProperties struct { // reading of a track that declares neither. deliveryTimeouts message.DeliveryTimeouts - // groupOrder is DEFAULT_PUBLISHER_GROUP_ORDER (§12.5) when it holds an - // allowed value, else zero. + // groupOrder is DEFAULT_PUBLISHER_GROUP_ORDER (§12.5), zero when omitted. + // The session closed on a value outside {1, 2}. groupOrder message.GroupOrder } // decodeTrackProperties parses the raw Track Properties block once and pulls // out the fields the relay acts on. A structural parse failure short-circuits -// to a parseErr that every accessor surfaces; per-property value violations -// (e.g. §12.6) are recorded on the matching field's error. +// to a parseErr that every accessor surfaces. The session has closed on a +// session-fatal value (§12.5, §12.6) before Properties reach here. func decodeTrackProperties(raw []byte) decodedProperties { pairs, err := message.ParseTrackProperties(raw) if err == nil { @@ -60,48 +57,32 @@ func decodeTrackProperties(raw []byte) decodedProperties { // branch here for each new property. switch kv.Type { case message.PropertyDynamicGroups: - d.dynamicGroups, d.dynamicGroupsErr = decodeDynamicGroups(kv.IntVal) + d.dynamicGroups = kv.IntVal == 1 case message.PropertyObjectDeliveryTimeout: d.deliveryTimeouts.Object = message.MillisecondTimeout(kv.IntVal) case message.PropertySubgroupDeliveryTimeout: d.deliveryTimeouts.Subgroup = message.MillisecondTimeout(kv.IntVal) case message.PropertyDefaultPublisherGroupOrder: - d.groupOrder = 0 - if kv.IntVal == uint64(message.GroupOrderAscending) || kv.IntVal == uint64(message.GroupOrderDescending) { - d.groupOrder = message.GroupOrder(kv.IntVal) + d.groupOrder = message.GroupOrderAscending + if kv.IntVal == uint64(message.GroupOrderDescending) { + d.groupOrder = message.GroupOrderDescending } } } return d } -// decodeDynamicGroups interprets a DYNAMIC_GROUPS value (§12.6): 0 is false, -// 1 is true, and anything greater is a PROTOCOL_VIOLATION so the caller can -// decline to act on it. -func decodeDynamicGroups(v uint64) (bool, error) { - switch v { - case 0: - return false, nil - case 1: - return true, nil - default: - return false, fmt.Errorf( - "relay: DYNAMIC_GROUPS value %d > 1 (§12.6 PROTOCOL_VIOLATION)", v) - } -} - // DynamicGroups reports whether the track advertised DYNAMIC_GROUPS=1 (§12.6), -// using the value decoded once when Properties was set. The error is a §12.6 -// PROTOCOL_VIOLATION (a DYNAMIC_GROUPS value > 1), or a structural failure -// parsing the Properties block; either way the §10.2.19 caller declines the -// NEW_GROUP_REQUEST rather than acting on it. +// using the value decoded once when Properties was set. The error is a +// structural failure parsing the Properties block; the §10.2.19 caller then +// declines the NEW_GROUP_REQUEST rather than acting on it. func (e *TrackEntry) DynamicGroups() (bool, error) { e.mu.RLock() defer e.mu.RUnlock() if e.decoded.parseErr != nil { return false, e.decoded.parseErr } - return e.decoded.dynamicGroups, e.decoded.dynamicGroupsErr + return e.decoded.dynamicGroups, nil } // DeliveryTimeouts returns the publisher's Track-level delivery timeouts (§8), @@ -125,7 +106,7 @@ func (e *TrackEntry) DeliveryTimeouts() message.DeliveryTimeouts { // DefaultGroupOrder is the publisher's Group Order preference, its // DEFAULT_PUBLISHER_GROUP_ORDER Track Property (§12.5): Ascending when it is // omitted ("If omitted, the publisher's preference is Ascending"), and when -// it is not an allowed value or the Properties are malformed. +// the Properties are malformed. func (e *TrackEntry) DefaultGroupOrder() message.GroupOrder { e.mu.RLock() defer e.mu.RUnlock() diff --git a/pkg/relay/internal/registry/subscription.go b/pkg/relay/internal/registry/subscription.go index 2f293aef..601879a3 100644 --- a/pkg/relay/internal/registry/subscription.go +++ b/pkg/relay/internal/registry/subscription.go @@ -322,20 +322,16 @@ func (u *UpstreamSub) Cancel(code moqt.StreamResetCode) { // The Forward State starts at 1: an omitted FORWARD means 1 (§10.2.18), and // the relay's upstream requests never carry it. // -// peerMayUpdate says whether the upstream publisher may send REQUEST_UPDATE: -// §10.9 allows it only from "The sender of a request", so true for an -// accepted PUBLISH and false for the relay's own SUBSCRIBE. +// broker owns the request stream's reads, set up by the caller: the +// [session.Subscription]'s own for the relay's SUBSCRIBE, so the session +// releases its Track Alias when it ends (§11.1). func NewUpstreamSub( id uint64, sess *session.Session, stream session.Stream, + broker *session.RequestBroker, trackAlias, requestID uint64, - peerMayUpdate bool, ) *UpstreamSub { - broker := sess.NewRequestBroker(stream) - broker.PeerMessages(peerMayUpdate, true) - // Only an accepted PUBLISH's publisher may update here (§10.9). - broker.UpdateScope(message.ScopeUpdateFromPublisher) return &UpstreamSub{ state: SubEstablished, ID: id, diff --git a/pkg/relay/internal/registry/subscription_test.go b/pkg/relay/internal/registry/subscription_test.go index 79c9b9da..df355832 100644 --- a/pkg/relay/internal/registry/subscription_test.go +++ b/pkg/relay/internal/registry/subscription_test.go @@ -37,7 +37,7 @@ func TestSubscription_BornEstablished(t *testing.T) { IsEstablished() bool IsTerminated() bool }{ - registry.NewUpstreamSub(1, nil, nil, 0, 0, false), + registry.NewUpstreamSub(1, nil, nil, nil, 0, 0), registry.NewDownstreamSub(1, nil, nil, 0), } { if !sub.IsEstablished() || sub.IsTerminated() { @@ -52,7 +52,7 @@ func TestSubscription_BornEstablished(t *testing.T) { // reports false. func TestSubscription_TerminateLatch(t *testing.T) { t.Parallel() - sub := registry.NewUpstreamSub(1, nil, nil, 0, 0, false) + sub := registry.NewUpstreamSub(1, nil, nil, nil, 0, 0) if !sub.Terminate() { t.Fatal("first Terminate returned false, want true") @@ -74,7 +74,7 @@ func TestSubscription_TerminateLatch(t *testing.T) { // and the relay's upstream requests never carry FORWARD. func TestSubscription_ForwardState(t *testing.T) { t.Parallel() - sub := registry.NewUpstreamSub(1, nil, nil, 0, 0, false) + sub := registry.NewUpstreamSub(1, nil, nil, nil, 0, 0) if got := sub.ForwardState(); got != 1 { t.Fatalf("initial ForwardState = %d, want 1 (§10.2.18 default)", got) } @@ -91,7 +91,7 @@ func TestSubscription_ForwardState(t *testing.T) { // TestUpstreamSub_FilterRoundTrip pins the upstream filter accessor pair. func TestUpstreamSub_FilterRoundTrip(t *testing.T) { t.Parallel() - sub := registry.NewUpstreamSub(7, nil, nil, 42, 0, false) + sub := registry.NewUpstreamSub(7, nil, nil, nil, 42, 0) if sub.GetFilter() != nil { t.Fatal("initial filter not nil") } @@ -214,7 +214,7 @@ func TestDownstreamSub_EffectiveStreamPriority(t *testing.T) { // - the final state is Terminated. func TestSubscription_ConcurrentTerminate(t *testing.T) { t.Parallel() - sub := registry.NewUpstreamSub(1, nil, nil, 0, 0, false) + sub := registry.NewUpstreamSub(1, nil, nil, nil, 0, 0) const goroutines = 32 var winners atomic.Int32 diff --git a/pkg/relay/param_values_test.go b/pkg/relay/param_values_test.go new file mode 100644 index 00000000..7de2052f --- /dev/null +++ b/pkg/relay/param_values_test.go @@ -0,0 +1,91 @@ +package relay_test + +import ( + "math" + "testing" + + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/moqt/session" + "github.com/floatdrop/moq-go/pkg/relay" +) + +// TestRelay_SessionFatalParamValuesClose: a request carrying a value the draft +// makes session-fatal closes the relay's session, on every path the relay +// serves: GROUP_ORDER outside {1, 2} on PUBLISH and FETCH and inside +// FILL_PARAMETERS (§10.2.8), a DYNAMIC_GROUPS or DEFAULT_PUBLISHER_GROUP_ORDER +// Track Property out of range (§12.5, §12.6), and a LOCATION_FILTER whose end Group overflows +// (§5.1.2, which the relay used to answer per request), on an opener and in a +// REQUEST_UPDATE. +func TestRelay_SessionFatalParamValuesClose(t *testing.T) { + t.Parallel() + badOrder := message.ByteParam(message.ParamGroupOrder, 5) + overflow := message.AbsoluteRangeFilter(message.Location{Group: math.MaxUint64}, 1) + for _, tc := range []struct { + name string + // send sends the violation from another goroutine; prepare, if set, + // runs first on the test goroutine and returns what send needs. + prepare func(t *testing.T, sess *session.Session) *session.Subscription + send func(t *testing.T, sess *session.Session, sub *session.Subscription) + }{ + {name: "GROUP_ORDER on PUBLISH", send: func(t *testing.T, sess *session.Session, _ *session.Subscription) { + _, _ = sess.Publish(t.Context(), &message.Publish{ + Namespace: ns("video"), Name: []byte("cam1"), TrackAlias: 1, Parameters: message.Parameters{badOrder}, + }) + }}, + {name: "GROUP_ORDER on FETCH", send: func(t *testing.T, sess *session.Session, _ *session.Subscription) { + _, _ = sess.Fetch(t.Context(), &message.Fetch{ + Namespace: ns("video"), Name: []byte("cam1"), Parameters: message.Parameters{badOrder}, + }) + }}, + {name: "GROUP_ORDER inside FILL_PARAMETERS", send: func(t *testing.T, sess *session.Session, _ *session.Subscription) { + _, _ = sess.Subscribe(t.Context(), &message.Subscribe{ + Namespace: ns("video"), Name: []byte("cam1"), + Parameters: message.Parameters{message.FillParametersParam(message.Parameters{badOrder})}, + }) + }}, + {name: "LOCATION_FILTER overflow on SUBSCRIBE", send: func(t *testing.T, sess *session.Session, _ *session.Subscription) { + _, _ = sess.Subscribe(t.Context(), &message.Subscribe{ + Namespace: ns("video"), Name: []byte("cam1"), Parameters: message.Parameters{overflow}, + }) + }}, + {name: "LOCATION_FILTER overflow on FETCH", send: func(t *testing.T, sess *session.Session, _ *session.Subscription) { + _, _ = sess.Fetch(t.Context(), &message.Fetch{ + Namespace: ns("video"), Name: []byte("cam1"), Parameters: message.Parameters{overflow}, + }) + }}, + {name: "DYNAMIC_GROUPS 5 in PUBLISH Track Properties", send: func(t *testing.T, sess *session.Session, _ *session.Subscription) { + _, _ = sess.Publish(t.Context(), &message.Publish{ + Namespace: ns("video"), Name: []byte("cam1"), TrackAlias: 1, + TrackProperties: message.AppendTrackProperties(trackProp(message.PropertyDynamicGroups, 5)), + }) + }}, + {name: "DEFAULT_PUBLISHER_GROUP_ORDER 3 in PUBLISH Track Properties", send: func(t *testing.T, sess *session.Session, _ *session.Subscription) { + _, _ = sess.Publish(t.Context(), &message.Publish{ + Namespace: ns("video"), Name: []byte("cam1"), TrackAlias: 1, + TrackProperties: message.AppendTrackProperties(trackProp(message.PropertyDefaultPublisherGroupOrder, 3)), + }) + }}, + { + name: "LOCATION_FILTER overflow in a REQUEST_UPDATE", + prepare: func(t *testing.T, sess *session.Session) *session.Subscription { + publishVideoTrack(t, dialAnotherClient(t, sess), "cam1", 1) + return subscribeCam1(t, sess) + }, + send: func(t *testing.T, sess *session.Session, sub *session.Subscription) { + _, _ = sess.UpdateRequest(t.Context(), sub, message.Parameters{overflow}) + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + sess, teardown := connectRelay(t, relay.Config{}) + defer teardown() + var sub *session.Subscription + if tc.prepare != nil { + sub = tc.prepare(t, sess) + } + go tc.send(t, sess, sub) + requireSessionClosed(t, sess, tc.name) + }) + } +} diff --git a/pkg/relay/publish_done_test.go b/pkg/relay/publish_done_test.go index 257404e9..5ac42f5c 100644 --- a/pkg/relay/publish_done_test.go +++ b/pkg/relay/publish_done_test.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "io" - "math" "testing" "time" @@ -241,9 +240,8 @@ func TestPublishDone_EndedSubscriptionStopsAtNextObject(t *testing.T) { }() // A request-scoped malformed update: REQUEST_ERROR, then UPDATE_FAILED. - _, err = subSess.UpdateRequest(t.Context(), subReq, - message.Parameters{message.AbsoluteRangeFilter(message.Location{Group: math.MaxUint64}, 1)}) - requireRejectedWithCode(t, err, moqt.RequestMalformedTrack) + _, err = subSess.UpdateRequest(t.Context(), subReq, message.Parameters{badRangeFilter()}) + requireRejectedWithCode(t, err, moqt.RequestInvalidFilter) close(rejected) // the upstream subgroup goes on, and its stream stays open if pd := awaitPublishDone(t, subReq); pd.StatusCode != moqt.PublishDoneUpdateFailed { @@ -393,12 +391,10 @@ func TestPublishDone_AfterFailedFill(t *testing.T) { t.Fatal("the object never reached the relay") } - // A five-field LOCATION_FILTER in FILL_PARAMETERS does not parse, which - // fails the fill after SUBSCRIBE_OK. + // An invalid Range Filter in FILL_PARAMETERS fails the fill after + // SUBSCRIBE_OK. subSess := dialAnotherClient(t, pubSess) - subReq := subscribeCam1(t, subSess, message.FillParametersParam(message.Parameters{ - message.BytesParam(message.ParamLocationFilter, []byte{0, 0, 0, 0, 0}), - })) + subReq := subscribeCam1(t, subSess, message.FillParametersParam(message.Parameters{badRangeFilter()})) ds, err := subSess.AcceptDataStream(t.Context()) if err != nil { t.Fatalf("AcceptDataStream (fill): %v", err) diff --git a/pkg/relay/request_goaway_test.go b/pkg/relay/request_goaway_test.go new file mode 100644 index 00000000..55baa78e --- /dev/null +++ b/pkg/relay/request_goaway_test.go @@ -0,0 +1,49 @@ +package relay_test + +import ( + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/relay" +) + +// TestRelay_RequestStreamGoaway: on a subscriber's request stream the relay +// (the server) closes the session for a second GOAWAY or for one carrying a +// New Session URI (§10.4), and tolerates a single plain one. +func TestRelay_RequestStreamGoaway(t *testing.T) { + t.Parallel() + uri := &message.Goaway{NewSessionURI: []byte("https://relay.example/moq")} + for _, tc := range []struct { + name string + sent []*message.Goaway + closes bool + }{ + {"second GOAWAY", []*message.Goaway{{}, {}}, true}, + {"New Session URI from a client", []*message.Goaway{uri}, true}, + {"one GOAWAY", []*message.Goaway{{}}, false}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + pubSess, teardown := connectRelay(t, relay.Config{}) + t.Cleanup(teardown) + publishVideoTrack(t, pubSess, "cam1", 1) + subSess := dialAnotherClient(t, pubSess) + sub := subscribeCam1(t, subSess) + for _, m := range tc.sent { + if err := message.Marshal(sub, m); err != nil { + t.Fatalf("write GOAWAY: %v", err) + } + } + if tc.closes { + requireSessionClosed(t, subSess, tc.name) + return + } + select { + case <-subSess.Done(): + t.Fatalf("relay closed the session on %s: %v", tc.name, subSess.Err()) + case <-time.After(100 * time.Millisecond): + } + }) + } +} diff --git a/pkg/relay/second_response_test.go b/pkg/relay/second_response_test.go new file mode 100644 index 00000000..da2914b0 --- /dev/null +++ b/pkg/relay/second_response_test.go @@ -0,0 +1,42 @@ +package relay_test + +import ( + "context" + "testing" + "time" + + "github.com/floatdrop/moq-go/pkg/moqt/message" + "github.com/floatdrop/moq-go/pkg/relay" +) + +// TestRelay_SecondResponseCloses: the relay sends no REQUEST_UPDATE on a +// forwarded PUBLISH, so a REQUEST_OK or REQUEST_ERROR the subscriber sends +// after its PUBLISH_OK is a second response, and the relay closes the session +// (§5.1). +func TestRelay_SecondResponseCloses(t *testing.T) { + t.Parallel() + for _, resp := range []message.Message{&message.RequestOK{}, &message.RequestError{}} { + t.Run(resp.Type().String()+" after PUBLISH_OK on a forwarded PUBLISH", func(t *testing.T) { + t.Parallel() + subSess, teardown := connectRelay(t, relay.Config{}) + t.Cleanup(teardown) + subscribeTracks(t, subSess, ns("video")) + publish(t, dialAnotherClient(t, subSess), &message.Publish{ + Namespace: ns("video", "cam7"), Name: []byte("rtp"), TrackAlias: 99, + }) + ctx, cancel := context.WithTimeout(t.Context(), 2*time.Second) + defer cancel() + req, err := subSess.AcceptRequest(ctx) + if err != nil { + t.Fatalf("AcceptRequest: %v", err) + } + if _, err := req.AcceptPublish(); err != nil { + t.Fatalf("AcceptPublish: %v", err) + } + if err := message.Marshal(req.Stream, resp); err != nil { + t.Fatalf("write second response: %v", err) + } + requireSessionClosed(t, subSess, "a second response to a forwarded PUBLISH") + }) + } +} diff --git a/pkg/relay/session_handler.go b/pkg/relay/session_handler.go index 5ab66fb7..36daee47 100644 --- a/pkg/relay/session_handler.go +++ b/pkg/relay/session_handler.go @@ -559,6 +559,7 @@ func readRequestStream( // done carries the fin result, so nothing else escapes to the heap. done := make(chan bool, 1) go func() { + var goaways session.RequestGoaways for { m, err := message.Parse(stream) if err != nil { @@ -577,6 +578,12 @@ func readRequestStream( done <- eof return } + // §10.4: a second GOAWAY, or one with a URI from a client, + // closed the session. + if g, ok := m.(*message.Goaway); ok && goaways.Received(sess, g) != nil { + done <- false + return + } if !onMsg(m) { done <- false return diff --git a/pkg/relay/session_publish_test.go b/pkg/relay/session_publish_test.go index 06a4187b..941111ce 100644 --- a/pkg/relay/session_publish_test.go +++ b/pkg/relay/session_publish_test.go @@ -5,7 +5,6 @@ import ( "testing" "time" - "github.com/floatdrop/moq-go/pkg/moqt" "github.com/floatdrop/moq-go/pkg/moqt/message" "github.com/floatdrop/moq-go/pkg/moqt/session" "github.com/floatdrop/moq-go/pkg/moqt/track" @@ -35,20 +34,23 @@ func TestPublish_AcceptedAndRegistered(t *testing.T) { } } -// TestPublish_DuplicateAliasRejected: reusing a Track Alias on the session for -// another track refuses the PUBLISH (§11.1). -func TestPublish_DuplicateAliasRejected(t *testing.T) { +// TestPublish_DuplicateAliasClosesSession: reusing the Track Alias of a +// published track on the session for another track closes the session with +// DUPLICATE_TRACK_ALIAS (§11.1). +func TestPublish_DuplicateAliasClosesSession(t *testing.T) { t.Parallel() clientSess, teardown := connectRelay(t, relay.Config{}) defer teardown() publishVideoTrack(t, clientSess, "cam1", 7) - _, err := clientSess.Publish(t.Context(), &message.Publish{ - Namespace: ns("video"), - Name: []byte("cam2"), - TrackAlias: 7, - }) - requireRejectedWithCode(t, err, moqt.RequestMalformedTrack) + go func() { + _, _ = clientSess.Publish(t.Context(), &message.Publish{ + Namespace: ns("video"), + Name: []byte("cam2"), + TrackAlias: 7, + }) + }() + requireSessionClosed(t, clientSess, "a PUBLISH reusing another track's alias") } // TestPublish_SavesLargestObjectFromPublish: LARGEST_OBJECT on an inbound diff --git a/pkg/relay/session_update_test.go b/pkg/relay/session_update_test.go index 1714d32b..26276007 100644 --- a/pkg/relay/session_update_test.go +++ b/pkg/relay/session_update_test.go @@ -2,7 +2,6 @@ package relay_test import ( "context" - "math" "testing" "time" @@ -84,13 +83,9 @@ func TestRequestUpdate_MalformedRejectedWithUpdateFailed(t *testing.T) { } defer subStream.Close() - // An AbsoluteRange filter whose end-group delta overflows the start group - // (§5.1.2) fails installSubscribeParams' filter validation. Unlike an - // out-of-range GROUP_ORDER/FORWARD, a bad filter stays request-scoped, so - // the relay answers REQUEST_ERROR rather than closing the session. - _, err = subSess.UpdateRequest(t.Context(), subStream, - message.Parameters{message.AbsoluteRangeFilter(message.Location{Group: math.MaxUint64}, 1)}) - requireRejectedWithCode(t, err, moqt.RequestMalformedTrack) + // An invalid Range Filter is refused per request (§5.1.4). + _, err = subSess.UpdateRequest(t.Context(), subStream, message.Parameters{badRangeFilter()}) + requireRejectedWithCode(t, err, moqt.RequestInvalidFilter) // §10.9: the failed update is followed by a PUBLISH_DONE with // UPDATE_FAILED on the same stream. diff --git a/pkg/relay/session_upstream_test.go b/pkg/relay/session_upstream_test.go index b509415e..4724145d 100644 --- a/pkg/relay/session_upstream_test.go +++ b/pkg/relay/session_upstream_test.go @@ -273,6 +273,36 @@ func TestSubscribe_LastDownstreamTearsDownUpstream(t *testing.T) { } } +// TestSubscribe_UpstreamAliasReusableAfterTeardown: once the relay ends its +// upstream subscription, the publisher may use its Track Alias for a different +// Track (§11.1) without the relay closing the session with +// DUPLICATE_TRACK_ALIAS. +func TestSubscribe_UpstreamAliasReusableAfterTeardown(t *testing.T) { + t.Parallel() + pubSess := namespacePublisher(t, relay.Config{}) + subscriptionEnded := acceptUpstreamSubscribe(t, pubSess, 77) + + subSess := dialAnotherClient(t, pubSess) + _ = subscribeCam1(t, subSess).Close() + select { + case <-subscriptionEnded: + case <-time.After(2 * time.Second): + t.Fatal("publisher's subscription still open 2s after the last downstream left") + } + + acceptUpstreamSubscribe(t, pubSess, 77) + if _, err := subSess.Subscribe(t.Context(), &message.Subscribe{ + Namespace: ns("video"), Name: []byte("cam2"), + }); err != nil { + t.Fatalf("Subscribe cam2 on the reused alias: %v", err) + } + select { + case <-pubSess.Done(): + t.Fatal("relay closed the publisher's session on a reused alias") + case <-time.After(50 * time.Millisecond): + } +} + // TestSubscribe_UpstreamRejects_PropagatesRejection: an upstream REQUEST_ERROR // code about the track passes downstream; one about the relay's own hop becomes // INTERNAL_ERROR (§10.6.2). The Retry Interval is kept either way. diff --git a/pkg/relay/subscribe_tracks_test.go b/pkg/relay/subscribe_tracks_test.go index fba88389..76abfcfa 100644 --- a/pkg/relay/subscribe_tracks_test.go +++ b/pkg/relay/subscribe_tracks_test.go @@ -253,17 +253,19 @@ func TestForwardedPublish_RepublishedTrackForwardedAgain(t *testing.T) { } // TestForwardedPublish_SubscribeTracksParametersValidated: SUBSCRIBE_TRACKS -// parameters are validated as on SUBSCRIBE (§10.20.1): a malformed -// LOCATION_FILTER is MALFORMED_TRACK. +// parameters are validated as on SUBSCRIBE (§10.20.1): a LOCATION_FILTER that +// does not parse closes the session (§1.4.3). func TestForwardedPublish_SubscribeTracksParametersValidated(t *testing.T) { t.Parallel() subSess, teardown := connectRelay(t, relay.Config{}) defer teardown() - _, err := subSess.SubscribeTracks(t.Context(), &message.SubscribeTracks{ - TrackNamespacePrefix: ns("video"), - Parameters: message.Parameters{message.BytesParam(message.ParamLocationFilter, []byte{0xFF})}, - }) - requireRejectedWithCode(t, err, moqt.RequestMalformedTrack) + go func() { + _, _ = subSess.SubscribeTracks(t.Context(), &message.SubscribeTracks{ + TrackNamespacePrefix: ns("video"), + Parameters: message.Parameters{message.BytesParam(message.ParamLocationFilter, []byte{0xFF})}, + }) + }() + requireSessionClosed(t, subSess, "a SUBSCRIBE_TRACKS with a LOCATION_FILTER that does not parse") } // A REQUEST_UPDATE on SUBSCRIBE_TRACKS applies to the PUBLISHes sent from then