waitForNotifyUserMediaSubs polls where the other five waits race an event

Open
  • Is something wrong?
#286 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
48/100
Issue type
Refactor
Clarity
Clearly specified
Activity status
Quiet
Tech stack
typescript
Domain
networking

Research direction

Start with lib/drivers/ddp.ts, Socket.subscribeAll, and the probe wait shape, then run the two named driver-suite cases that cover in-flight resubscription and partial subscriptions. Done means the wait is connection-shaped, preserves the listed boolean behavior, uses the current connection, and leaves no timer or listener after settlement.

Written by the indexing model from the issue text.

Description

needs-triage

lib/drivers/ddp.ts holds six bounded waits. Five race the thing they wait for against a Deadline. DDPDriver.waitForNotifyUserMediaSubs is the exception: it polls Socket.subscriptions every 100ms, carries settled and inFlight flags, and keeps running on a connection that has already been replaced.

The poll does not do what its docblock says

The docblock says it polls when the subscriptions are not yet present, "e.g. immediately after reopenNow". But a Reopen does not clear Socket.subscriptions — ADR-0004 states that a Reopen forgets nothing, and the driver suite pins it. After a Reopen the entries are already there and the first synchronous attempt wins.

The only state in which it actually polls is one where the consuming app's own subscribeNotifyUser has not been acked yet. That is a race with the app's login sequence, not with the reconnect.

inFlight guards nothing

resubscribe cannot reject (see #285), so the first attempt that finds both subscriptions always settles the promise. The inFlight = false reset is unreachable.

It sits on the wrong class

Socket.subscribe is documented as the sole owner of subscriptions. The method lives on DDPDriver and reaches through this.ddp three times: for the subscription map, for config.timeout, and to call Socket.subscribe directly — because DDPDriver.subscribe reshapes its arguments and drops the subscription id. That last reach carries a comment explaining the bypass rather than removing the need for it.

What it does is a filtered Socket.subscribeAll, and subscribeAll already lives on Socket.

Other consequences of not being connection-shaped
  • It has no close / connecting / disconnected listener, so it keeps resubscribing across a connection that has already gone away. ADR-0003's amendment establishes that a wait ends with the connection it was issued on.
  • A resubscribe on a healthy connection overwrites the entry in the map while the previous onEvent listener stays registered on the emitter, producing duplicate stream events for that id.
  • userId is captured once at entry, but it is a mutable field. A login as a different user during the wait leaves the predicate matching the previous user's subscriptions.
  • Its false covers three different outcomes — no userId, the Deadline expiring, and a failed resubscribe — and a caller cannot tell them apart. "Never logged in" and "the server was slow" want opposite responses.
  • After the Deadline settles the promise false, the in-flight resubscribe continues and can still mutate the subscription map.
Proposed shape

Look the subscriptions up once, then race Promise.all of the two subscribe calls — each already settles on the server's ready ack — against the Deadline and against the events that end the connection. That is the same shape as probe. The interval, attempt, poll and inFlight all disappear.

Move it to Socket, beside subscribeAll, which removes all three reaches through this.ddp. DDPDriver keeps a thin wrapper holding userId and the boolean at the boundary.

Constraint

The method must keep resolving a boolean and must never reject. It is not declared on IDriver; Rocket.Chat.ReactNative reaches it through the untyped client.ddp escape hatch described in #284, so a change of shape would not be caught by TypeScript in either repo.

Behaviour to preserve
  • No userId resolves false synchronously, sends nothing and schedules no timers
  • Both media-signal and media-calls are required; one alone never resolves
  • Resubscribes go out on the current connection, not a replaced one
  • Each resubscribe reuses the stored subscription id and params
  • Resolves true only after the server acks both with ready
  • At most one outstanding resubscribe per subscription
  • Resolves false at the Deadline, default config.timeout, overridable by argument
  • Never rejects
  • Leaves no timer or listener behind once settled
Tests

Two cases in the driver suite pin the poll itself and would be rewritten — the one asserting no second resubscribe while one is in flight, and the one stepping the timers 100ms at a time until both subscriptions appear. What each is really asserting survives: at most one resubscribe per subscription, and half the subscriptions is not enough to resolve.

Dominant language
TypeScript
Stars
146
Forks
101
No merged PRs in 30d

Getting set up

This project ships no dev container, Dockerfile or contributing guide, so setting up is up to you: start from its README, and see our first-contribution guide for the general steps.

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from RocketChat/Rocket.Chat.js.SDK

All issues in RocketChat/Rocket.Chat.js.SDK

Similar issues

More TypeScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.