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

Move syncthing to new execution model #1267

Merged
merged 20 commits into from
Jun 5, 2024

Conversation

MorningLightMountain713
Copy link
Contributor

@MorningLightMountain713 MorningLightMountain713 commented Apr 5, 2024

What this pull does

  • Adds ability to stop syncthingService (doesn't get used, it's just the ability).
  • Abortable axios requests
  • Implements axiosCache for syncthingService, caches apikey and axios instance for 15 minutes.
  • Updates fluxController to be able to loop a passed in "runner" function.
  • Updates the syncthingService to use the new serviceHelper command runner.
  • Updates the syncthingService to use the fluxController loop runner.
  • Adds 30 syncthingService tests (non comprehensive).
  • Sets executable bit on helper scripts so they can be run directly.
  • Adds userconfig global variable to eslintrc so can remove all the userconfig exceptions from each file.

standalone

Have added the ability to run the syncthingService as a standalone. Just added this stanza at the end of the module:

// similar to python's if __name__ == "__main__", allows module
// to be run as standalone, useful for testing
if (require.main === module) {
  startSyncthingSentinel();

  process.stdin.on('data', async (data) => {
    const cmd = data.toString().trim();
    if (cmd === 'start') startSyncthingSentinel();
    if (cmd === 'stop') await stopSyncthingSentinel();
  });
}

This is helpful for testing. (It won't run under normal circumstances or during tests) You can start it like this (from the repo root):

NODE_CONFIG_DIR=./ZelBack/config FLUX_APIPORT=16187 node ZelBack/src/services/syncthingService.js

You can then type "stop" and hit enter and it will stop the service. You can then type "start" and it will start it back up again.

Notes

To simplify some of the functions, and maintain seperation of concerns, have started splitting the actual business logic from the express middlewares. For example:

   app.get('/syncthing/deviceid', cache('30 seconds'), (req, res) => {
-    syncthingService.getDeviceID(req, res);
+    syncthingService.getDeviceIdApi(req, res);
   });

Then the getDeviceIdApi function calls getDevice - this means getDevice doesn't have to be aware / create all the api responses. It can focus on returning the deviceId. This simplifies tests etc.

You may have noticed I've started refactoring some of the functions to better adhere to camel case. From a previous role, we learnt the hard way not to capitalize acronyms. This leads to confusion around where one word starts and another word ends. It's more explicit and predictable to only capitalize the first letter of an acronym. Eg instead of deviceID we have deviceId (I appreciate syncthing is using deviceID in their config and have left this)

A contrived example:

startServerHTMLTLS - this isn't obvious at first glance what it's supposed to be. Whereas, startServerHtmlTls - there is no ambiguity around what this function does.

I've been running this branch on a node for a week and all is well, however, due to the syncthing changes, would be good to get this on some other development nodes and try to break it just to make sure I've covered everything.

Node specifier

Something else to note, have started using the full node: specifier for nodeJS inbuilt modules. E.g. instead of const fs = require('fs); would use const fs = require('node:fs'); This does a couple of things, makes it obvious that we are indeed using a nodeJS built in module, it also skips the require cache - so if there was a file named fs.js, it would still use the node built in module.

@MorningLightMountain713
Copy link
Contributor Author

MorningLightMountain713 commented Apr 5, 2024

A note on the node: specifier, I'm going to remove this. The ability to use the node: specifier was introduced @ v14.18.0. Looking on FluxStats - the oldest version we have on the network is v14.18.1. However, that may not take into account older versions that haven't updated flux and reporting the node version?

I think it's a bit close, and if they were on older versions would break the node. Will leave this until we are confident that ALL nodes are on nodeJS versions > v14.18.0

@MorningLightMountain713
Copy link
Contributor Author

MorningLightMountain713 commented Apr 5, 2024

Looking at the node specifier, I already added it in the prior version when doing the serviceHelper. (This probably shouldn't have been added)

So if there is anyone on an older version... it will be broken already

Here is what happens on node 14.17.x and prior:

Welcome to Node.js v14.17.6.
Type ".help" for more information.
> const serviceHelper = require('./ZelBack/src/services/serviceHelper')
Uncaught Error: Cannot find module 'node:util'
Require stack:
- /Users/davew/code/flux/flux/ZelBack/src/services/serviceHelper.js
- <repl>
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:885:15)
    at Function.Module._load (internal/modules/cjs/loader.js:730:27)
    at Module.require (internal/modules/cjs/loader.js:957:19)
    at require (internal/modules/cjs/helpers.js:88:18) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/Users/davew/code/flux/flux/ZelBack/src/services/serviceHelper.js',
    '<repl>'
  ]
}
> serviceHelper
Uncaught ReferenceError: serviceHelper is not defined

Here is node 14.18.0:

Welcome to Node.js v14.18.0.
Type ".help" for more information.
> const serviceHelper = require('./ZelBack/src/services/serviceHelper')
undefined
>

@MorningLightMountain713
Copy link
Contributor Author

Have removed all instances of node: from requires for tests and in serviceHelper / syncthingService

@MorningLightMountain713
Copy link
Contributor Author

All tests passing

Screenshot 2024-04-05 at 1 53 48 PM

@MorningLightMountain713
Copy link
Contributor Author

MorningLightMountain713 commented Apr 5, 2024

This can't get merged until nodes are on at least node v15.0.0

AbortController wasn't introduced until the above version.

Based on this - I will undo the latest commit to get rid of the node specifier - as it can't be merged anyway as is

Copy link

codecov bot commented Apr 5, 2024

Codecov Report

Attention: Patch coverage is 84.15301% with 29 lines in your changes are missing coverage. Please review.

Project coverage is 48.44%. Comparing base (19731c7) to head (9b43cfd).

Current head 9b43cfd differs from pull request most recent head 1f2f1a7

Please upload reports for the commit 1f2f1a7 to get more accurate results.

Files Patch % Lines
ZelBack/src/services/syncthingService.js 86.92% 20 Missing ⚠️
ZelBack/src/services/appsService.js 0.00% 9 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           development    #1267      +/-   ##
===============================================
+ Coverage        47.72%   48.44%   +0.71%     
===============================================
  Files               50       47       -3     
  Lines            15667    15526     -141     
===============================================
+ Hits              7477     7521      +44     
+ Misses            8190     8005     -185     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MorningLightMountain713
Copy link
Contributor Author

I found a decent polyfill for AbortController on nodeJS 14.x.

  • It has approx 10M weekly downloads.
  • We would only need the package node-abort-controller until all nodes are > 14.x.
  • This means we can merge this code, one step closer to being able to remove node-cmd
  • Means we can also merge the other request (once I rebase it onto develop)

With the polyfill - this will work on all nodes.

Of note, 14.81.1 does have AbortController if you enable Node experimental features, however, easier / better to use a polyfill as we can enable it for just the nodes that need it.

@MorningLightMountain713
Copy link
Contributor Author

I'm just testing this on a live node now. Will let it run for a week or so and see how it goes

@MorningLightMountain713
Copy link
Contributor Author

This has been running for the last 48hours on one of my nodes. Looks good.

@Cabecinha84
Copy link
Member

Will merge to dev after tuesday release! Nice work!

Copy link
Member

@Cabecinha84 Cabecinha84 left a comment

Choose a reason for hiding this comment

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

Ack, nice job!

@Cabecinha84 Cabecinha84 merged commit 6062c34 into development Jun 5, 2024
2 checks passed
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.

2 participants