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

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Mar 13, 2024

Closes EXEC-304

Overview

Migrating MQTT connectivity logic to discovery-client simplifies app-shell connectionStore manager logic (maybe), reduces bug surface, and gives us connection retry logic relatively for free.

Most importantly, we will not depend on components to express interest in a broker and topics, which implicitly requires all components and notify helper functions in the app to behave appropriately (not re-render like crazy).

Note that there's some browser layer logic that is no longer relevant (like UNSUBSCRIBE actions), but these still need to be here until app-shell-odd is refactored. While the app still dispatches these actions, they don't actively do anything.

App-shell notifications behave as follows:

  • Broker connections are added or forcefully removed based on health status changes reported by discovery-client.
  • Subscriptions are handled "lazily", in which a component must express interest in a topic before a subscription request is made (no network request will be made if already subscribed tot he topic).
  • Unsubscribe requests only occur if an "unsubscribe" flag is received from the broker. That means the only thing we actively unsubscribe from at the moment is "/runs/:runId".
  • Pending subs and unsubs are used to prevent unnecessary network and broker load.
  • A blocked port event can only be reported a maximum of one time per robot per app session (no change here).

Test Plan

  • Push refactor(robot-server): Utilize unsubscribe flags for dynamic topics #14620 to a robot.
  • The easiest way to test this is to do a setInterval(() => {console.log(connectionStore)}, 1000) within notify.ts as this gives direct feedback on what's happening.
  • Start the app in dev mode, you should see a bunch of app-shell logs in which the app tries to connect to all robots that are available. Note that on our network, there are a lot of OT-2s and Flexes on 7.0.2, so it's fine to see a bunch of failed connections. Some should still go through through.
  • Click on the Devices page. In the app-shell logs, you should see a bunch of subscribe actions going through the IPC and the app-shell should report successful subscribes.
  • Turn off a robot. Whenever the discovery-client polls and updates the available robots tab, you should see that robot be removed from the connection store. You won't actually see the "closed connection" log, because the robot is completely offline.
  • Run a protocol and cancel it. I don't remember if it's when you restart a run or as soon as the cancel process is complete, but at some point the app-shell logs should say "successfully unsubscribed..." with the topic string that matches the run ID (this is the only topic with an unsubscribe flag for now).

Risk assessment

medium-ish. This is a pretty big refactor of how notifications work for the desktop app.

@mjhuff mjhuff requested review from sfoster1 and a team March 13, 2024 15:27
@mjhuff mjhuff requested a review from a team as a code owner March 13, 2024 15:27
@mjhuff mjhuff requested review from brenthagen and removed request for a team and brenthagen March 13, 2024 15:27
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand what this is doing, but it's getting really confusing. There's a lot of free-variable associative stores at the file scope, robots can be in multiple of them at once, there's several different pending states.

I think a lot of this will be cleared up and also testable if you consolidate some of this into a single object or closure with a couple methods that move robots between states atomically. Then have a file-scope instance of that, and have the various actions and callbacks use it to move robots around. At that point, it's pure logic and testable.

app-shell/src/notify.ts Outdated Show resolved Hide resolved
app-shell/src/notify.ts Outdated Show resolved Hide resolved
app-shell/src/notify.ts Outdated Show resolved Hide resolved
app-shell/src/notify.ts Outdated Show resolved Hide resolved
@mjhuff
Copy link
Contributor Author

mjhuff commented Mar 19, 2024

While testing I realized that I didn't account for the common scenario of discovery-client reporting multiple IPs for one robot (ethernet, wifi). We don't want multiple broker connections from the same app, since that's extremely inefficient. So the solution:

  • IPs can be associated based on "name" (which is really robot name). This is guaranteed to be network unique, meaning that two IPs with the same name point to the same robot (and therefore MQTT broker).
  • We take the first MDNS reported IP and try connecting to it for that robot. During in person convo w/ Seth, we brought up the idea of prioritizing connection based on selecting an IP with an identical subnet to the host. However, that introduces another layer of complexity - how do we decide to handle connections while MDNS is initially polling? Do we not connect to any broker for the first X seconds? Do we connect and then disconnect if we find an identical subnet? I think for now, it's fine to assume that if a robot is connected to multiple networks, it is free to use any of those networks.
  • If a network is port blocked, we will try connecting on alternative networks. Ex, if we connect to a robot's wifi network before ethernet, return an ECONNREFUSED during the connection ACK, we will try the ethernet network (if all networks fail, we just default to polling as before).
  • Restarting a robot doesn't cause any funky issues. I've tested this thoroughly.

Anyway, I think this is ready for review again. Let me know your thoughts. I will add testing once we've decided we're ok with this (hopefully final) iteration of the connection manager.

@mjhuff mjhuff marked this pull request as ready for review March 19, 2024 13:49
@mjhuff mjhuff requested review from sfoster1 and a team March 19, 2024 13:49
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More details in comments, but broad-strokes stuff:

  • Since we appear to be focused on keeping one connection per broker, does it make sense to have the connection store have robotNameByIp: Record<string, string> and hostsByRobotName: Record<string, HostData>? that way we don't have to do a search for matching hostnames every time.

app-shell/src/notifications/store.ts Outdated Show resolved Hide resolved
app-shell/src/notifications/store.ts Outdated Show resolved Hide resolved
app-shell/src/notifications/store.ts Show resolved Hide resolved
app-shell/src/notifications/store.ts Show resolved Hide resolved
app-shell/src/notifications/store.ts Outdated Show resolved Hide resolved
@sfoster1
Copy link
Member

Uh I submitted that too early I'll have followups

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Fundamentally looks good to me but I think there's some refactors we can do for clarity.

app-shell/src/notifications/connect.ts Outdated Show resolved Hide resolved
@mjhuff mjhuff force-pushed the app_shell-migrate-notify-to-discovery-client branch from c93702a to af12fbc Compare March 20, 2024 23:24
@mjhuff mjhuff force-pushed the app_shell-migrate-notify-to-discovery-client branch from af12fbc to 2fb099c Compare March 20, 2024 23:28
@mjhuff mjhuff merged commit 7a750ff into edge Mar 21, 2024
20 checks passed
@mjhuff mjhuff deleted the app_shell-migrate-notify-to-discovery-client branch March 21, 2024 18:50
mjhuff added a commit that referenced this pull request Mar 25, 2024
Closes EXEC-319

This is the app-shell-odd equivalent of the app-shell refactor, #14648. It's similar to the app-shell logic, but significantly simpler, since we don't have to manage multiple robots, worry about localhost port blocking, and multiple IPs per robot. The real change lies in the initial connect and final disconnect on app shutdown. Otherwise, the changes are primarily in the ConnectionStore. Because the app no longer utilizes unsubscribe actions in any capacity, we can safely remove those references.
Carlos-fernandez pushed a commit that referenced this pull request May 20, 2024
Closes EXEC-319

This is the app-shell-odd equivalent of the app-shell refactor, #14648. It's similar to the app-shell logic, but significantly simpler, since we don't have to manage multiple robots, worry about localhost port blocking, and multiple IPs per robot. The real change lies in the initial connect and final disconnect on app shutdown. Otherwise, the changes are primarily in the ConnectionStore. Because the app no longer utilizes unsubscribe actions in any capacity, we can safely remove those references.
Carlos-fernandez pushed a commit that referenced this pull request Jun 3, 2024
Closes EXEC-319

This is the app-shell-odd equivalent of the app-shell refactor, #14648. It's similar to the app-shell logic, but significantly simpler, since we don't have to manage multiple robots, worry about localhost port blocking, and multiple IPs per robot. The real change lies in the initial connect and final disconnect on app shutdown. Otherwise, the changes are primarily in the ConnectionStore. Because the app no longer utilizes unsubscribe actions in any capacity, we can safely remove those references.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants