Skip to content

Commit 80cfe7e

Browse files
authored
fix(app, app-shell, app-shell-odd): block initial HTTP request until successful MQTT subscription (#15094)
Closes EXEC-429 Currently, the desktop app/ODD attempt to address the "missed update problem" in the following way: while we subscribe to a topic, we simultaneously GET whatever equivalent HTTP resource we just subscribed to. However, there's definitely a world (albeit a very small one) in which we receive the HTTP response, a server update occurs, the server publishes, and then we successfully subscribe to a topic. In this world, we've missed the update event. Solve for this by simply refetching right after the client subscribe ACKs. We still keep the initial fetch on mount to keep latency low.
1 parent d9aa18f commit 80cfe7e

File tree

5 files changed

+46
-16
lines changed

5 files changed

+46
-16
lines changed

app-shell-odd/src/notifications/deserialize.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,10 @@ export function sendDeserialized(
3131
} catch {} // Prevents shell erroring during app shutdown event.
3232
}
3333

34+
export function sendDeserializedRefetch(topic: NotifyTopic): void {
35+
sendDeserialized(topic, { refetch: true })
36+
}
37+
3438
export function sendDeserializedGenericError(topic: NotifyTopic): void {
3539
sendDeserialized(topic, FAILURE_STATUSES.ECONNFAILED)
3640
}

app-shell-odd/src/notifications/subscribe.ts

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,11 @@
11
import mqtt from 'mqtt'
22

33
import { connectionStore } from './store'
4-
import { sendDeserialized, sendDeserializedGenericError } from './deserialize'
4+
import {
5+
sendDeserialized,
6+
sendDeserializedGenericError,
7+
sendDeserializedRefetch,
8+
} from './deserialize'
59
import { notifyLog } from './notifyLog'
610

711
import type { NotifyTopic } from '@opentrons/app/src/redux/shell/types'
@@ -30,8 +34,8 @@ export function subscribe(topic: NotifyTopic): Promise<void> {
3034
if (client == null) {
3135
return Promise.reject(new Error('Expected hostData, received null.'))
3236
}
33-
34-
if (
37+
// The first time the client wants to subscribe on a robot to a particular topic.
38+
else if (
3539
!connectionStore.isActiveSub(topic) &&
3640
!connectionStore.isPendingSub(topic)
3741
) {
@@ -44,13 +48,15 @@ export function subscribe(topic: NotifyTopic): Promise<void> {
4448
})
4549
)
4650
.catch((error: Error) => notifyLog.debug(error.message))
47-
} else {
48-
void waitUntilActiveOrErrored('subscription', topic).catch(
49-
(error: Error) => {
51+
}
52+
// The client is either already subscribed or the subscription is currently pending.
53+
else {
54+
void waitUntilActiveOrErrored('subscription', topic)
55+
.then(() => sendDeserializedRefetch(topic))
56+
.catch((error: Error) => {
5057
notifyLog.debug(error.message)
5158
sendDeserializedGenericError(topic)
52-
}
53-
)
59+
})
5460
}
5561
})
5662
.catch((error: Error) => {
@@ -74,6 +80,8 @@ export function subscribe(topic: NotifyTopic): Promise<void> {
7480
connectionStore
7581
.setSubStatus(topic, 'subscribed')
7682
.catch((error: Error) => notifyLog.debug(error.message))
83+
84+
sendDeserializedRefetch(topic)
7785
}
7886
}
7987
}

app-shell/src/notifications/deserialize.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,14 @@ export function sendDeserialized({
3333
} catch {} // Prevents shell erroring during app shutdown event.
3434
}
3535

36+
export function sendDeserializedRefetch(ip: string, topic: NotifyTopic): void {
37+
sendDeserialized({
38+
ip,
39+
topic,
40+
message: { refetch: true },
41+
})
42+
}
43+
3644
export function sendDeserializedGenericError(
3745
ip: string,
3846
topic: NotifyTopic

app-shell/src/notifications/subscribe.ts

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,11 @@
11
import mqtt from 'mqtt'
22

33
import { connectionStore } from './store'
4-
import { sendDeserialized, sendDeserializedGenericError } from './deserialize'
4+
import {
5+
sendDeserialized,
6+
sendDeserializedGenericError,
7+
sendDeserializedRefetch,
8+
} from './deserialize'
59
import { notifyLog } from './notifyLog'
610

711
import type { NotifyTopic } from '@opentrons/app/src/redux/shell/types'
@@ -36,8 +40,8 @@ export function subscribe(ip: string, topic: NotifyTopic): Promise<void> {
3640
if (client == null) {
3741
return Promise.reject(new Error('Expected hostData, received null.'))
3842
}
39-
40-
if (
43+
// The first time the client wants to subscribe on a robot to a particular topic.
44+
else if (
4145
!connectionStore.isActiveSub(robotName, topic) &&
4246
!connectionStore.isPendingSub(robotName, topic)
4347
) {
@@ -50,16 +54,20 @@ export function subscribe(ip: string, topic: NotifyTopic): Promise<void> {
5054
})
5155
)
5256
.catch((error: Error) => notifyLog.debug(error.message))
53-
} else {
57+
}
58+
// The client is either already subscribed or the subscription is currently pending.
59+
else {
5460
void waitUntilActiveOrErrored({
5561
connection: 'subscription',
5662
ip,
5763
robotName,
5864
topic,
59-
}).catch((error: Error) => {
60-
notifyLog.debug(error.message)
61-
sendDeserializedGenericError(ip, topic)
6265
})
66+
.then(() => sendDeserializedRefetch(ip, topic))
67+
.catch((error: Error) => {
68+
notifyLog.debug(error.message)
69+
sendDeserializedGenericError(ip, topic)
70+
})
6371
}
6472
})
6573
.catch((error: Error) => {
@@ -81,6 +89,8 @@ export function subscribe(ip: string, topic: NotifyTopic): Promise<void> {
8189
connectionStore
8290
.setSubStatus(ip, topic, 'subscribed')
8391
.catch((error: Error) => notifyLog.debug(error.message))
92+
93+
sendDeserializedRefetch(ip, topic)
8494
}
8595
}
8696
}

app/src/resources/useNotifyService.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ export function useNotifyService<TData, TError = Error>({
5656

5757
React.useEffect(() => {
5858
if (shouldUseNotifications) {
59-
// Always fetch on initial mount.
59+
// Always fetch on initial mount to keep latency as low as possible.
6060
setRefetch('once')
6161
appShellListener({
6262
hostname,

0 commit comments

Comments
 (0)