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.

Sign up for GitHub

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

Jordigh/add websocket #973

Merged
merged 2 commits into from
May 17, 2024
+47 −3
Merged

Jordigh/add websocket #973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
019f9f6
debugging: add a separate start:debug-brk target
jordigh May 9, 2024
d0a494c
BootProbes: add a websocket probe
jordigh May 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions app/client/models/AdminChecks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,15 @@ It is good practice not to run Grist as the root user.
'reachable': {
info: `
The main page of Grist should be available.
`
},

'websockets': {
// TODO: add a link to https://support.getgrist.com/self-managed/#how-do-i-run-grist-on-a-server
info: `
Websocket connections need HTTP 1.1 and the ability to pass a few
extra headers in order to work. Sometimes a reverse proxy can
interfere with these requirements.
`
},
};
Expand Down
3 changes: 2 additions & 1 deletion app/common/BootProbe.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ export type BootProbeIds =
'host-header' |
'sandboxing' |
'system-user' |
'authentication'
'authentication' |
'websockets'
;

export interface BootProbeResult {
Expand Down
33 changes: 33 additions & 0 deletions app/server/lib/BootProbes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { removeTrailingSlash } from 'app/common/gutil';
import { expressWrap, jsonErrorHandler } from 'app/server/lib/expressWrap';
import { GristServer } from 'app/server/lib/GristServer';
import * as express from 'express';
import WS from 'ws';
import fetch from 'node-fetch';

/**
Expand Down Expand Up @@ -59,6 +60,7 @@ export class BootProbes {
this._probes.push(_hostHeaderProbe);
this._probes.push(_sandboxingProbe);
this._probes.push(_authenticationProbe);
this._probes.push(_webSocketsProbe);
this._probeById = new Map(this._probes.map(p => [p.id, p]));
}
}
Expand Down Expand Up @@ -105,6 +107,37 @@ const _homeUrlReachableProbe: Probe = {
}
};

const _webSocketsProbe: Probe = {
id: 'websockets',
name: 'Can we open a websocket with the server',
apply: async (server, req) => {
jordigh marked this conversation as resolved.
Show resolved Hide resolved
return new Promise((resolve) => {
const url = new URL(server.getHomeUrl(req));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

url.protocol = (url.protocol === 'https:') ? 'wss:' : 'ws:';
const ws = new WS.WebSocket(url.href);
const details: Record<string, any> = {
url,
};
ws.on('open', () => {
ws.send('Just nod if you can hear me.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

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}

Copy link
Contributor Author

@jordigh jordigh May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@jordigh jordigh May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

resolve({
success: true,
details,
});
ws.close();
});
ws.on('error', (ev) => {
details.error = ev.message;
resolve({
success: false,
details,
});
ws.close();
});
});
}
};

const _statusCheckProbe: Probe = {
id: 'health-check',
name: 'Is an internal health check passing',
Expand Down
3 changes: 2 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@
"repository": "git://github.com/gristlabs/grist-core.git",
"scripts": {
"start": "sandbox/watch.sh",
"start:debug": "NODE_INSPECT=1 sandbox/watch.sh",
"start:debug": "NODE_INSPECT=--inspect sandbox/watch.sh",
"start:debug-brk": "NODE_INSPECT=--inspect-brk sandbox/watch.sh",
"install:python": "buildtools/prepare_python.sh",
"install:python2": "buildtools/prepare_python2.sh",
"install:python3": "buildtools/prepare_python3.sh",
Expand Down
2 changes: 1 addition & 1 deletion sandbox/watch.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,6 @@ tsc --build -w --preserveWatchOutput $PROJECT &
css_files="app/client/**/*.css"
chokidar "${css_files}" -c "bash -O globstar -c 'cat ${css_files} > static/bundle.css'" &
webpack --config $WEBPACK_CONFIG --mode development --watch &
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 &
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this change do? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@fflorent fflorent May 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.


wait