feat(app-shell-odd): Use sd-notify #12266
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is the official npm package for the node bindings of sd_notify, which is how you tell systemd that you're ready to run.
This will let systemd have a better understanding of when the app is actually ready. It's TBD what happens when you do this on something other than a linux machine where libsystemd is available. We may need a workaround for dev tasks. Closes RCORE-683
I think this should work.
Codecov Report
@@ Coverage Diff @@
## edge #12266 +/- ##
==========================================
+ Coverage 73.55% 73.81% +0.26%
==========================================
Files 1477 2199 +722
Lines 48563 60921 +12358
Branches 2980 6341 +3361
==========================================
+ Hits 35719 44970 +9251
- Misses 12390 14489 +2099
- Partials 454 1462 +1008
Flags with carried forward coverage won't be shown. Click here to find out more.
|
shlokamin
approved these changes
Mar 10, 2023
Comment on lines
+63
to
+65
| }, | ||
| "optionalDependencies": { | ||
| "sd-notify": "2.8.0" |
There was a problem hiding this comment.
shouldnt this be a regular dependency?
There was a problem hiding this comment.
It won't actually install correctly on not-linux - this is a thin wrapper around system libsystemd
Comment on lines
+87
to
+88
| ipcMain.once('dispatch', () => { | ||
| sdNotify.sendStatus('started') |
There was a problem hiding this comment.
i think this should only send once since its in the startup fn and i assume this gets garbage collected and that event listener stops but we should make sure
There was a problem hiding this comment.
since it uses once doesn't that mean it'll only be one time?
sfoster1
added a commit
that referenced
this pull request
Mar 16, 2023
This is a fixup of #12266 ( ab85171 ) and more detail can be found there. This fixes the issue I should have remembered that we really can't use node extensions because they don't get cross-compiled correctly, so now we just shell out to the command-line executable. Requires some different flags (specifically https://github.com/Opentrons/oe-core/pull/63/files#diff-ec32fa0cac503403c1384b437fe44635c404ad5616d1711595f5c4c5b82073b8R14 ) but works fine once you do that.
sfoster1
added a commit
to Opentrons/oe-core
that referenced
this pull request
Mar 16, 2023
It is now a notify service. Requires Opentrons/opentrons#12266
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
sd-notify is a utility for a program to communicate back to the systemd process manager that started it. It can be used in a variety of ways; the one that is most critical is to positively assert that it has started OK. Since this is a function call you can make any time you want to, this lets your program define when it has "started" on its own terms.
The app, especially on the ODD, takes a long time to start - chrome takes a long time to start, and then we load up all our stuff. Currently, systemd considers us "started" as soon as the shell script that invokes chrome exits, which is long before anything useful is rendered. By making ourselves a
notifyservice (in Opentrons/oe-core#63) and putting the notify at the time we receive the first ipc message, which indicates that the frontend is up, we can delay that "ready" status until we're actually rendering.This can then be used as a criterion in systemd to take down a loading screen.
TBD
Test Plan