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

Reconnect the HTTP client websocket automatically #1254

Merged
merged 12 commits into from
May 28, 2024
12 changes: 4 additions & 8 deletions web/src/App.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,10 @@ import { useProduct } from "./context/product";
import { INSTALL, STARTUP } from "~/client/phase";
import { BUSY } from "~/client/status";

import { DBusError, If, Installation } from "~/components/core";
import { ServerError, If, Installation } from "~/components/core";
import { Loading } from "./components/layout";
import { useInstallerL10n } from "./context/installerL10n";

// D-Bus connection attempts before displaying an error.
const ATTEMPTS = 3;

/**
* Main application component.
*
Expand All @@ -43,7 +40,7 @@ const ATTEMPTS = 3;
*/
function App() {
const client = useInstallerClient();
const { attempt } = useInstallerClientStatus();
const { error } = useInstallerClientStatus();
const { products } = useProduct();
const { language } = useInstallerL10n();
const [status, setStatus] = useState(undefined);
Expand Down Expand Up @@ -75,9 +72,8 @@ function App() {
}, [client, setPhase, setStatus]);

const Content = () => {
if (!client || !products) {
return (attempt > ATTEMPTS) ? <DBusError /> : <Loading />;
}
if (error) return <ServerError />;
if (!products) return <Loading />;

if ((phase === STARTUP && status === BUSY) || phase === undefined || status === undefined) {
return <Loading />;
Expand Down
204 changes: 197 additions & 7 deletions web/src/client/http.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,23 @@
* @return {void}
*/

/**
* Enum for the WebSocket states.
*
*
*/

const SocketStates = Object.freeze({
CONNECTED: 0,
CONNECTING: 1,
CLOSING: 2,
CLOSED: 3,
UNRECOVERABLE: 4,
});

const MAX_ATTEMPTS = 15;
const ATTEMPT_INTERVAL = 1000;

/**
* Agama WebSocket client.
*
Expand All @@ -38,11 +55,70 @@ class WSClient {
* @param {URL} url - Websocket URL.
*/
constructor(url) {
this.client = new WebSocket(url.toString());
this.client.onmessage = (event) => {
this.url = url.toString();

this.handlers = {
error: [],
close: [],
open: [],
events: []
};

this.reconnectAttempts = 0;
this.client = this.buildClient();
}

wsState() {
const state = this.client.readyState;
if ((state !== SocketStates.CONNECTED) && (this.reconnectAttempts >= MAX_ATTEMPTS)) return SocketStates.UNRECOVERABLE;

return state;
}

isRecoverable() {
return (this.wsState() !== SocketStates.UNRECOVERABLE);
}

isConnected() {
return (this.wsState() === SocketStates.CONNECTED);
}

buildClient() {
const client = new WebSocket(this.url);
client.onopen = () => {
console.log("Websocket connected");
this.reconnectAttempts = 0;
clearTimeout(this.timeout);

return this.dispatchOpenEvent();
};

client.onmessage = (event) => {
this.dispatchEvent(event);
};
this.handlers = [];

client.onclose = () => {
console.log(`WebSocket closed`);
this.dispatchCloseEvent();
this.timeout = setTimeout(() => this.connect(this.reconnectAttempts + 1), ATTEMPT_INTERVAL);
};

client.onerror = (e) => {
console.error("WebSocket error:", e);
this.dispatchErrorEvent();
};

return client;
}

connect(attempt = 0) {
this.reconnectAttempts = attempt;
if (attempt > MAX_ATTEMPTS) {
console.log("Max number of WebSocket connection attempts reached.");
return;
}
console.log(`Reconnecting WebSocket(attempt: ${attempt})`);
this.client = this.buildClient();
}

/**
Expand All @@ -55,10 +131,60 @@ class WSClient {
* @return {RemoveFn}
*/
onEvent(func) {
this.handlers.push(func);
this.handlers.events.push(func);
return () => {
const position = this.handlers.events.indexOf(func);
if (position > -1) this.handlers.events.splice(position, 1);
};
}

/**
* Registers a handler for close socket.
*
* The handler is executed when the socket is close.
*
* @param {(object) => void} func - Handler function to register.
* @return {RemoveFn}
*/
onClose(func) {
this.handlers.close.push(func);

return () => {
const position = this.handlers.indexOf(func);
if (position > -1) this.handlers.splice(position, 1);
const position = this.handlers.close.indexOf(func);
if (position > -1) this.handlers.close.splice(position, 1);
};
}

/**
* Registers a handler for open socket.
*
* The handler is executed when the socket is open.
* @param {(object) => void} func - Handler function to register.
* @return {RemoveFn}
*/
onOpen(func) {
this.handlers.open.push(func);

return () => {
const position = this.handlers.open.indexOf(func);
if (position > -1) this.handlers.open.splice(position, 1);
};
}

/**
* Registers a handler for socket errors.
*
* The handler is executed when an error is reported by the socket.
*
* @param {(object) => void} func - Handler function to register.
* @return {RemoveFn}
*/
onError(func) {
this.handlers.error.push(func);

return () => {
const position = this.handlers.error.indexOf(func);
if (position > -1) this.handlers.error.splice(position, 1);
};
}

Expand All @@ -71,7 +197,34 @@ class WSClient {
*/
dispatchEvent(event) {
const eventObject = JSON.parse(event.data);
this.handlers.forEach((f) => f(eventObject));
this.handlers.events.forEach((f) => f(eventObject));
}

/**
* @private
*
* Dispatchs a close event by running all its handlers.
*/
dispatchCloseEvent() {
this.handlers.close.forEach((f) => f());
}

/**
* @private
*
* Dispatchs an error event by running all its handlers.
*/
dispatchErrorEvent() {
this.handlers.error.forEach((f) => f());
}

/**
* @private
*
* Dispatchs a close event by running all its handlers.
*/
dispatchOpenEvent() {
this.handlers.open.forEach((f) => f());
}
}

Expand Down Expand Up @@ -169,6 +322,43 @@ class HTTPClient {
return response;
}

/**
* Registers a handler for being called when the socket is closed
*
* @param {() => void} func - Handler function to register.
* @return {RemoveFn} - Function to remove the handler.
*/
onClose(func) {
return this.ws.onClose(() => {
func();
});
}

/**
*
* Registers a handler for being called when there is some error in the socket
*
* @param {(event: Object) => void} func - Handler function to register.
* @return {RemoveFn} - Function to remove the handler.
*/
onError(func) {
return this.ws.onError((event) => {
func(event);
});
}

/**
* Registers a handler for being called when the socket is opened
*
* @param {(event: Object) => void} func - Handler function to register.
* @return {RemoveFn} - Function to remove the handler.
*/
onOpen(func) {
return this.ws.onOpen((event) => {
func(event);
});
}

/**
* Registers a handler for a given type of events.
*
Expand Down
24 changes: 9 additions & 15 deletions web/src/client/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ import { HTTPClient } from "./http";
* @typedef {object} InstallerClient
* @property {L10nClient} l10n - localization client.
* @property {ManagerClient} manager - manager client.
* @property {Monitor} monitor - service monitor.
* property {Monitor} monitor - service monitor. (FIXME)
* @property {NetworkClient} network - network client.
* @property {ProductClient} product - product client.
* @property {SoftwareClient} software - software client.
Expand All @@ -46,7 +46,9 @@ import { HTTPClient } from "./http";
* @property {() => Promise<Issues>} issues - issues from all contexts.
* @property {(handler: IssuesHandler) => (() => void)} onIssuesChange - registers a handler to run
* when issues from any context change. It returns a function to deregister the handler.
* @property {() => Promise<boolean>} isConnected - determines whether the client is connected
* @property {() => boolean} isConnected - determines whether the client is connected
* @property {() => boolean} isRecoverable - determines whether the client is recoverable after disconnected
* @property {(handler: () => void) => (() => void)} onConnect - registers a handler to run
* @property {(handler: () => void) => (() => void)} onDisconnect - registers a handler to run
* when the connection is lost. It returns a function to deregister the
* handler.
Expand Down Expand Up @@ -122,15 +124,8 @@ const createClient = (url) => {
};
};

const isConnected = async () => {
// try {
// await manager.getStatus();
// return true;
// } catch (e) {
// return false;
// }
return true;
};
const isConnected = () => client.ws?.isConnected() || false;
const isRecoverable = () => !!client.ws?.isRecoverable();

return {
l10n,
Expand All @@ -145,10 +140,9 @@ const createClient = (url) => {
issues,
onIssuesChange,
isConnected,
onDisconnect: (handler) => {
return () => { };
},
// onDisconnect: (handler) => monitor.onDisconnect(handler),
isRecoverable,
onConnect: (handler) => client.ws.onOpen(handler),
onDisconnect: (handler) => client.ws.onClose(handler),
};
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,19 +28,19 @@ import { locationReload } from "~/utils";

const ErrorIcon = () => <Icon name="error" className="icon-xxxl" />;

function DBusError() {
function ServerError() {
return (
// TRANSLATORS: page title
<Page icon="problem" title={_("D-Bus Error")}>
<Page icon="problem" title={_("Agama Error")}>
<Center>
<EmptyState variant="xl">
<EmptyStateHeader
titleText={_("Cannot connect to D-Bus")}
titleText={_("Cannot connect to Agama server")}
headingLevel="h2"
icon={<EmptyStateIcon icon={ErrorIcon} />}
/>
<EmptyStateBody>
{_("Could not connect to the D-Bus service. Please, check whether it is running.")}
{_("Could not connect to the Agama server. Please, check whether it is running.")}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would say this part of the message is not useful at all. The first sentence is basically a repetition of the bigger "Cannot connect to...". And, as a user, how can I check whether the service is running?

By now, I would remove the first sentence. About the second part, we should link to some page explaining how to proceed. But that's out of the scope of this card.

Copy link
Contributor Author

@teclator teclator May 28, 2024

Choose a reason for hiding this comment

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

I agree, maybe we could maintain the first part and in the second part show some info about the different errors although by now we are only reporting it in case of a websocket connection error..

</EmptyStateBody>
</EmptyState>
</Center>
Expand All @@ -55,4 +55,4 @@ function DBusError() {
);
}

export default DBusError;
export default ServerError;
Original file line number Diff line number Diff line change
Expand Up @@ -24,19 +24,19 @@ import { screen } from "@testing-library/react";
import { plainRender } from "~/test-utils";

import * as utils from "~/utils";
import { DBusError } from "~/components/core";
import { ServerError } from "~/components/core";

jest.mock("~/components/core/Sidebar", () => () => <div>Agama sidebar</div>);

describe("DBusError", () => {
it("includes a generic D-Bus connection problem message", () => {
plainRender(<DBusError />);
screen.getByText(/Could not connect to the D-Bus service/i);
describe("ServerError", () => {
it("includes a generic server problem message", () => {
plainRender(<ServerError />);
screen.getByText(/Could not connect to the Agama server/i);
});

it("calls location.reload when user clicks on 'Reload'", async () => {
jest.spyOn(utils, "locationReload").mockImplementation(utils.noop);
const { user } = plainRender(<DBusError />);
const { user } = plainRender(<ServerError />);
const reloadButton = await screen.findByRole("button", { name: /Reload/i });
await user.click(reloadButton);
expect(utils.locationReload).toHaveBeenCalled();
Expand Down
Loading
Loading