Jordigh/add websocket #973

jordigh · 2024-05-09T20:52:15Z

Adds a simple websocket test.

I tested this on my test instance by enabling and disabling headers in nginx. The test passed and failed accordingly.

--inspect-brk is useful if you want to debug something during startup

This self-test isn't perfect because we're running it from the backend instead of the frontend. Conceivably the backend might have trouble resolving its own url. Eventually we should move this test or something like it to something that executes in the frontend.

jordigh · 2024-05-09T20:53:17Z

app/client/models/AdminChecks.ts

+extra headers in order to work. Sometimes a reverse proxy can
+interfere with these requirements. See <a
+href="https://support.getgrist.com/self-managed/#how-do-i-run-grist-on-a-server">this
+documentation</a> for more explanation.


This isn't correct, but I just realised that grainjs is our own thing. Is there an easy way to put a link here? I'm not sure how to cross-reference otherwise.

Maybe makeLinks from links.ts?

That ui2018 in the module path doesn't inspire confidence. Is that really the best way? 😕

No, it's the easy way.

The best way would be to change the details to be DomContents instead of strings. GristTooltips.ts is a good example of largely static blocks of text comprised of DOM nodes.

It'll require a bit of refactoring since details are currently strings, but it shouldn't be much if you're up for it.

One thing to watch out for would be import paths. The rule of thumb is client code (e.g. anything dependent on grainjs) goes in client/; server code goes in server/ or gen-server/; and anything that's shared across client and server goes in common/.

I think it might be worth it, might try this tomorrow. I may like to add more links for other explanations here. Thanks!

Can you defer that bit? AdminChecks changes in a parallel PR. Put small "add more explanation link to websocket probe" card in the kanban for now?

Right-o, will do.

Spoffy · 2024-05-10T15:30:14Z

app/client/ui/AdminPanel.ts

@@ -145,6 +165,7 @@ export class AdminPanel extends Disposable {
 }

 private _buildSandboxingNotice() {
+ // TODO: reconcile with AdminChecks text for sandboxing.


Flagging this TODO in case it's something to sort before merging :)

Spoffy · 2024-05-10T17:36:25Z

sandbox/watch.sh

-NODE_PATH=_build:_build/stubs:_build/ext nodemon ${NODE_INSPECT:+--inspect} --delay 1 -w _build/app/server -w _build/app/common _build/stubs/app/server/server.js &
+NODE_PATH=_build:_build/stubs:_build/ext nodemon ${NODE_INSPECT} --delay 1 -w _build/app/server -w _build/app/common _build/stubs/app/server/server.js &


What does this change do? :)

It's unrelated, but I explain it in the best place to hide sensitive secrets: in the commit message.

In case you didn't know (and sorry if you did), --inspect will start and run a Node process that you can attach a debugger, whereas --inspect-brk starts the process but immediately sets a breakpoint at the entry point of execution. The latter is helpful if you want to start debugging a process from the very beginning. In the case of Grist, I wanted to debug initialisation to understand how the server starts listening for websocket connexions.

To add something about that: using yarn start:debug is not perfect as it requires to close the debugger for letting nodemon restart the node process when you change some piece of code… That's probably a nodemon / node limitation.

Unless there's something new since the last time I used this command.

fflorent

Few remarks, with the hope they can be relevant :).

app/server/lib/BootProbes.ts

fflorent · 2024-05-10T17:49:41Z

app/server/lib/BootProbes.ts

+ name: 'Can we open a websocket with the server',
+ apply: async (server, req) => {
+ return new Promise((resolve) => {
+ const url = new URL(server.getHomeUrl(req));


Unless I am wrong, with #915, this will have to be changed to getHomeInternalUrl. Or does it mean to reach the reverse-proxy, to test whether it is configured to handle correctly the websockets?

My comment will probably not require any action in any case, though, especially if you merge before the above PR.

Yes, the purpose was to test the reverse proxy.

This test has a bigger problem, though: it runs all in the backend. We plan to fix this and other connectivity tests by making them run in the frontend.

Thanks! I was indeed wondering if that was not a better idea to handle that client-side :)

fflorent · 2024-05-10T17:59:14Z

app/server/lib/BootProbes.ts

+ url,
+ };
+ ws.on('open', () => {
+ ws.send('Just nod if you can hear me.');


Does it have any effect? It looks like the server expects serialized JSON.

In case you want to know whether the server can respond, I noticed that the server sends a message right after the connection is open, with content like this one:

{"serverVersion":"unknown","settings":{},"type":"clientConnect","clientId":"8381953b21f26c76","profile":{"email":"[email protected]","name":"You"},"needReload":false}

Maybe I should augment or change the test to not be complete until a response message is received. I don't particularly care about the shape of the messages, just that a message happens.

I see. What are the benefit of sending this message? Is there something that would prevent the promise to be resolved the line after? Or is this for printing something in the server logs for the administrators?

I reasoned that if for some reason a message couldn't be sent, the error event would be triggered. I'm not sure if that's how the WS implementation works, though.

georgegevoian

Thanks @jordigh!

jordigh added 2 commits May 9, 2024 16:43

debugging: add a separate start:debug-brk target

019f9f6

--inspect-brk is useful if you want to debug something during startup

jordigh commented May 9, 2024

View reviewed changes

paulfitz force-pushed the paulfitz/smoosh branch from d40457b to d33c157 Compare May 9, 2024 21:37

jordigh force-pushed the jordigh/add-websocket branch from 0c9bed9 to 556a197 Compare May 10, 2024 17:33

Spoffy approved these changes May 10, 2024

View reviewed changes

fflorent reviewed May 10, 2024

View reviewed changes

jordigh force-pushed the jordigh/add-websocket branch 2 times, most recently from a771297 to 31bd2dd Compare May 13, 2024 14:51

georgegevoian approved these changes May 13, 2024

View reviewed changes

paulfitz force-pushed the paulfitz/smoosh branch 2 times, most recently from d564d1d to 58de132 Compare May 14, 2024 18:18

jordigh force-pushed the jordigh/add-websocket branch from 31bd2dd to 52ecefb Compare May 14, 2024 21:04

paulfitz force-pushed the paulfitz/smoosh branch from c377e0f to 54257c2 Compare May 16, 2024 19:34

jordigh force-pushed the jordigh/add-websocket branch from 52ecefb to d0a494c Compare May 17, 2024 13:10

jordigh merged commit 07b80b1 into paulfitz/smoosh May 17, 2024

jordigh deleted the jordigh/add-websocket branch May 17, 2024 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jordigh/add websocket #973

Jordigh/add websocket #973

jordigh commented May 9, 2024 •

edited

jordigh May 9, 2024

georgegevoian May 9, 2024

jordigh May 9, 2024

georgegevoian May 9, 2024 •

edited

jordigh May 9, 2024

paulfitz May 9, 2024

jordigh May 9, 2024

Spoffy May 10, 2024

Spoffy May 10, 2024

jordigh May 10, 2024

fflorent May 10, 2024 •

edited

fflorent left a comment

fflorent May 10, 2024

jordigh May 10, 2024

fflorent May 10, 2024

fflorent May 10, 2024

jordigh May 10, 2024 •

edited

fflorent May 10, 2024

jordigh May 10, 2024 •

edited

georgegevoian left a comment

		NODE_PATH=_build:_build/stubs:_build/ext nodemon ${NODE_INSPECT:+--inspect} --delay 1 -w _build/app/server -w _build/app/common _build/stubs/app/server/server.js &
		NODE_PATH=_build:_build/stubs:_build/ext nodemon ${NODE_INSPECT} --delay 1 -w _build/app/server -w _build/app/common _build/stubs/app/server/server.js &

Jordigh/add websocket #973

Jordigh/add websocket #973

Conversation

jordigh commented May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgegevoian May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fflorent May 10, 2024 • edited

Choose a reason for hiding this comment

fflorent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordigh May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordigh May 10, 2024 • edited

Choose a reason for hiding this comment

georgegevoian left a comment

Choose a reason for hiding this comment

jordigh commented May 9, 2024 •

edited

georgegevoian May 9, 2024 •

edited

fflorent May 10, 2024 •

edited

jordigh May 10, 2024 •

edited

jordigh May 10, 2024 •

edited