Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(app-shell): Migrate desktop app notify connectivity to discovery-client #14648

Merged
merged 39 commits into from
Mar 21, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
421958b
update constants
mjhuff Mar 12, 2024
9b07898
redo notify to support new connectivity and unsubscribes
mjhuff Mar 12, 2024
c519ee1
add the callback to discovery
mjhuff Mar 12, 2024
4704772
clean up unsubscribe
mjhuff Mar 13, 2024
f0de47d
try/catch the webcontents send to prevent an error on app close
mjhuff Mar 13, 2024
417b402
we don't need to pass in browserWindow now
mjhuff Mar 13, 2024
2fa5cd8
denest and consolidate everything
mjhuff Mar 13, 2024
c5e2ee2
let's try a new structure here
mjhuff Mar 13, 2024
2b10635
everything in its own file
mjhuff Mar 13, 2024
02d9d77
clean up index.ts
mjhuff Mar 13, 2024
4504ad4
clean up deserialize.ts
mjhuff Mar 13, 2024
1f63b4a
clean up connect.ts
mjhuff Mar 13, 2024
fd1c44d
clean up subscribe
mjhuff Mar 13, 2024
780e0c1
random small cleanup
mjhuff Mar 13, 2024
b14bd61
perhaps closures are the poor man's class
mjhuff Mar 14, 2024
ce0c2b4
pycharm fun
mjhuff Mar 14, 2024
3d5635b
spruce up deserialize
mjhuff Mar 14, 2024
da27bb1
spruce up index
mjhuff Mar 14, 2024
466d03d
spruce up connect
mjhuff Mar 15, 2024
d0ca3da
spruce ub subscribe
mjhuff Mar 15, 2024
fc73504
spruce up unsubscribe
mjhuff Mar 15, 2024
9a00811
some cleanup
mjhuff Mar 15, 2024
75939dc
log.ts -> notifyLog.ts
mjhuff Mar 15, 2024
f403610
promise
mjhuff Mar 15, 2024
9b2cb25
random cleanup
mjhuff Mar 15, 2024
785744c
DRY up the sendToBrowser logic
mjhuff Mar 15, 2024
cb7f8d1
use the top level name property from discovery-client to prevent redu…
mjhuff Mar 18, 2024
107efcd
disconnect from hosts as they become unreachable
mjhuff Mar 18, 2024
dcd45a7
cleanup
mjhuff Mar 18, 2024
5731094
properly handle associated host connection & disconnection
mjhuff Mar 18, 2024
954288f
we mean ip, not hostname
mjhuff Mar 18, 2024
1d6800e
P R O M I S I F Y (and add errors)
mjhuff Mar 18, 2024
264ee27
fix a couple booleans
mjhuff Mar 18, 2024
e2f5485
let's add robotName to error messages
mjhuff Mar 18, 2024
dcd0b5b
retry on other IPs if current is port blocked
mjhuff Mar 18, 2024
c25f0ce
cleanup connectivity
mjhuff Mar 19, 2024
2fb099c
feedback
mjhuff Mar 20, 2024
5313b98
a few random fixes
mjhuff Mar 21, 2024
d18d2f8
some tests
mjhuff Mar 21, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
properly handle associated host connection & disconnection
  • Loading branch information
mjhuff committed Mar 18, 2024
commit 573109480b25b87074f58b4a57c196c9fc9837ff
36 changes: 30 additions & 6 deletions app-shell/src/notifications/connect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,32 @@ export function addNewRobotsToConnectionStore(
healthyRobots: RobotData[]
): Promise<void> {
return new Promise(() => {
const newRobots = healthyRobots.filter(({ hostname, robotName }) =>
connectionStore.isHostNewlyDiscovered(hostname, robotName)
)
const newRobots = healthyRobots.filter(({ hostname, robotName }) => {
const isNewHostname = connectionStore.isHostnameNewlyDiscovered(hostname)
const isHostnameAssociatedWithKnownRobot = connectionStore.isAssociatedWithExistingHostData(
robotName
)
// Not a new robot, but a new IP for a robot present in connectionStore.
if (isNewHostname && isHostnameAssociatedWithKnownRobot) {
// Pass until the next discovery-client poll so the current connection can resolve.
if (!connectionStore.isAssociatedHostnameConnected(hostname)) {
return false
}
// Hostname is reachable on an associated IP. Do not connect on this IP.
if (connectionStore.isAssociatedHostnameReachable(hostname)) {
connectionStore.associateWithExistingHostData(hostname, robotName)
return false
}
// The robot isn't reachable on existing IPs.
// Mark this IP as a new connection to see if the robot is reachable on this IP.
else {
connectionStore.deleteAllAssociatedHostsGivenRobotName(robotName)
return true
}
} else {
return isNewHostname
}
})
newRobots.forEach(({ hostname, robotName }) => {
connectionStore.setPendingHost(hostname, robotName)
connectAsync(`mqtt://${hostname}`)
Expand Down Expand Up @@ -163,7 +186,6 @@ function establishListeners({

client.on('end', () => {
notifyLog.debug(`Closed connection to ${hostname}`)
connectionStore.deleteHost(hostname)
})

client.on('disconnect', packet => {
Expand All @@ -182,8 +204,10 @@ export function closeConnectionsForcefullyFor(
return hosts.map(hostname => {
const client = connectionStore.getClient(hostname)
return new Promise<void>((resolve, reject) => {
client?.end(true, {})
connectionStore.deleteHost(hostname)
if (client != null) {
client.end(true, {})
}
connectionStore.deleteAllAssociatedHostsGivenHostname(hostname)
resolve()
})
})
Expand Down
121 changes: 78 additions & 43 deletions app-shell/src/notifications/store.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* eslint-disable @typescript-eslint/no-dynamic-delete */
import type mqtt from 'mqtt'
import head from 'lodash/head'

import { FAILURE_STATUSES } from '../constants'

Expand All @@ -8,23 +9,20 @@ import type { BrowserWindow } from 'electron'

type FailedConnStatus = typeof FAILURE_STATUSES[keyof typeof FAILURE_STATUSES]

interface IHosts {
interface HostData {
robotName: string
client: mqtt.MqttClient | null
subscriptions: Set<NotifyTopic>
pendingSubs: Set<NotifyTopic>
pendingUnsubs: Set<NotifyTopic>
unreachableStatus: FailedConnStatus | null
}

/**
* Manages the internal state of MQTT connections to various robot hosts.
*/
class ConnectionStore {
private hosts: Record<string, IHosts> = {}

private unreachableHosts: Record<string, FailedConnStatus> = {}

readonly seenRobotNames = new Set<string>()
private hosts: Record<string, HostData> = {}

private browserWindow: BrowserWindow | null = null

Expand All @@ -46,10 +44,10 @@ class ConnectionStore {
* for analytics reasons. Afterward, a generic "ECONNFAILED" is returned.
*/
public getFailedConnectionStatus(hostname: string): FailedConnStatus | null {
if (hostname in this.unreachableHosts) {
const failureStatus = this.unreachableHosts[hostname]
if (hostname in this.hosts) {
const failureStatus = this.hosts[hostname].unreachableStatus
if (failureStatus === FAILURE_STATUSES.ECONNREFUSED) {
this.unreachableHosts[hostname] = FAILURE_STATUSES.ECONNFAILED
this.hosts[hostname].unreachableStatus = FAILURE_STATUSES.ECONNFAILED
}
return failureStatus
} else {
Expand All @@ -61,21 +59,32 @@ class ConnectionStore {
return Object.keys(this.hosts)
}

public getAssociatedHostnamesFromHostname(hostname: string): string[] {
const robotName = this.hosts[hostname].robotName
return Object.keys(this.hosts).filter(
hostname => this.hosts[hostname].robotName === robotName
)
}

public getAssociatedHostnamesFromRobotName(robotName: string): string[] {
return Object.keys(this.hosts).filter(
hostname => this.hosts[hostname].robotName === robotName
)
}

public setBrowserWindow(window: BrowserWindow): void {
this.browserWindow = window
}

public setPendingHost(hostname: string, robotName: string): void {
if (!this.seenRobotNames.has(robotName)) {
if (!(hostname in this.hosts)) {
this.seenRobotNames.add(robotName)
this.hosts[hostname] = {
robotName,
client: null,
subscriptions: new Set(),
pendingSubs: new Set(),
pendingUnsubs: new Set(),
}
if (!this.isAssociatedWithExistingHostData(robotName)) {
this.hosts[hostname] = {
robotName,
client: null,
subscriptions: new Set(),
pendingSubs: new Set(),
pendingUnsubs: new Set(),
unreachableStatus: null,
}
}
}
Expand All @@ -94,14 +103,11 @@ class ConnectionStore {
*/
public setFailedToConnectHost(hostname: string, error: Error): void {
if (hostname in this.hosts) {
delete this.hosts[hostname]
}
if (!(hostname in this.unreachableHosts)) {
const errorStatus = error.message.includes(FAILURE_STATUSES.ECONNREFUSED)
? FAILURE_STATUSES.ECONNREFUSED
: FAILURE_STATUSES.ECONNFAILED

this.unreachableHosts[hostname] = errorStatus
this.hosts[hostname].unreachableStatus = errorStatus
}
}

Expand Down Expand Up @@ -139,26 +145,55 @@ class ConnectionStore {
}
}

public deleteHost(hostname: string): void {
if (hostname in this.hosts) {
this.seenRobotNames.delete(this.hosts[hostname].robotName)
delete this.hosts[hostname]
}
if (hostname in this.unreachableHosts) {
delete this.unreachableHosts[hostname]
/**
*
* @description Creates a new hosts entry for a given hostname with HostData that is a reference to an existing
* host's HostData. This occurs when two hostnames reported by discovery-client actually point to the same robot.
*/
public associateWithExistingHostData(
hostname: string,
robotName: string
): void {
const associatedHost = Object.values(this.hosts).find(
hostData => hostData.robotName === robotName
)
if (associatedHost != null) {
this.hosts[hostname] = associatedHost
}
}

public isHostNewlyDiscovered(hostname: string, name: string): boolean {
if (this.seenRobotNames.has(name)) {
return false
} else if (hostname in this.hosts) {
return false
} else if (hostname in this.unreachableHosts) {
return false
} else {
return true
}
// Deleting associated hosts does not prevent the connection from re-establishing on an associated host if an
// associated host is discoverable.
public deleteAllAssociatedHostsGivenHostname(hostname: string): void {
const associatedHosts = this.getAssociatedHostnamesFromHostname(hostname)
associatedHosts.forEach(hostname => {
delete this.hosts[hostname]
})
}

public deleteAllAssociatedHostsGivenRobotName(robotName: string): void {
const associatedHosts = this.getAssociatedHostnamesFromRobotName(robotName)
associatedHosts.forEach(hostname => {
delete this.hosts[hostname]
})
}

public isHostnameNewlyDiscovered(hostname: string): boolean {
return hostname in this.hosts
}

public isAssociatedWithExistingHostData(robotName: string): boolean {
return this.getAssociatedHostnamesFromRobotName(robotName).length != null
}

public isAssociatedHostnameReachable(hostname: string): boolean {
const associatedRobots = this.getAssociatedHostnamesFromHostname(hostname)
return this.isHostReachable(head(associatedRobots) as string)
}

public isAssociatedHostnameConnected(hostname: string): boolean {
const associatedRobots = this.getAssociatedHostnamesFromHostname(hostname)
return this.isHostConnected(head(associatedRobots) as string)
}

public isHostConnected(hostname: string): boolean {
Expand Down Expand Up @@ -197,10 +232,10 @@ class ConnectionStore {
}

public isHostReachable(hostname: string): boolean {
if (hostname in this.unreachableHosts) {
return false
if (hostname in this.hosts) {
return this.hosts[hostname].unreachableStatus == null
} else {
return hostname in this.hosts
return false
}
}
}
Expand Down