#818

HotspotManager's implicit state machine is spread across 17 mutable fields

Openmoehamadeopened 3mo ago

Follow-up from #808, #811 and #812. Not urgent, and not a bug report — a note about why those PRs took as many review rounds as they did.

Observation

HotspotManager currently carries 17 private var fields:

channel, wakeLock, wifiLock, currentGroup, callback, isStarting,
hasNotifiedStarted, isReceiverRegistered, savedSsid, savedPassword,
lastP2pState, createdActiveGroup, groupCreationPending,
recordedGeneratedName, teardownCallback, teardownInProgress, ownedGroupName

Five of those were added across #811 alone, each to answer a question an earlier fix had made askable:

FieldAdded because
createdActiveGroupremoval must not touch another app's group
groupCreationPendinga submitted createGroup can land after a stop
recordedGeneratedNamebelow Q the marker is only learned from the first poll
teardownCallbackthe Aware hold must outlive an asynchronous teardown
teardownInProgressa detached channel means two different things

Why it matters

Nearly every review finding on those PRs had the same shape: two of these fields disagreed, or one was read as meaning something it does not. A few examples:

  • ownedGroupName == null meant both "no group is ours" and "we have not learned the name yet" — states needing opposite handling on a recovery path.
  • channel == null meant both "never initialised" and "already being torn down".
  • hotspotManager == null in the ViewModel was standing in for "we owe an Aware release".

Each was fixed correctly and in isolation, but the underlying shape is a state machine whose states are implied by combinations of independent booleans. The number of reachable combinations grows faster than the guards do, which is why the findings kept arriving.

Suggestion

Replace the lifecycle flags with an explicit sealed state — roughly Idle, Starting, CreatingGroup, Hosting, TearingDown — carrying the data each state actually needs (the channel, the group name, the pending completion). Illegal combinations then stop being representable rather than being guarded against one at a time.

HotspotStartupPolicy is a useful precedent from #808: pulling the retry and preflight decisions into a pure, Android-free object made them directly testable, and none of the subsequent findings landed in it.

Worth noting the credential and lock fields are incidental here — the cluster that causes trouble is the lifecycle one.

🤖 Generated with Claude Code

Activity

0

No activity yet.