From 447d21045b791e1e2dca38bc9fd3ee169e25e1a3 Mon Sep 17 00:00:00 2001 From: Heikki Hellgren Date: Fri, 26 Jan 2024 09:31:06 +0200 Subject: [PATCH] fix: signal disconnect loop on server start this fixes the disconnect loop when the server has just started by waiting for the connection to open before subsribing to events. also includes other improvements: - rename types to interfaces - allow to input single receiver for signal - use discovery api to check for upgrade path instead hard coded one Signed-off-by: Heikki Hellgren --- .changeset/gorgeous-ways-applaud.md | 8 ++++++ packages/backend/src/plugins/signals.ts | 1 + plugins/signals-backend/README.md | 1 + plugins/signals-backend/api-report.md | 3 ++ plugins/signals-backend/src/plugin.ts | 4 ++- .../src/service/SignalManager.test.ts | 6 ++-- .../src/service/SignalManager.ts | 16 +++++++---- .../src/service/router.test.ts | 11 +++++++- plugins/signals-backend/src/service/router.ts | 12 +++++--- .../src/service/standaloneServer.ts | 3 +- plugins/signals-node/api-report.md | 6 ++-- .../signals-node/src/DefaultSignalService.ts | 4 +-- plugins/signals-node/src/SignalService.ts | 4 +-- plugins/signals-node/src/types.ts | 2 +- plugins/signals-react/api-report.md | 15 ++++++---- plugins/signals-react/src/api/SignalApi.ts | 11 ++++++-- plugins/signals/src/api/SignalClient.ts | 28 +++++++++---------- 17 files changed, 89 insertions(+), 46 deletions(-) create mode 100644 .changeset/gorgeous-ways-applaud.md diff --git a/.changeset/gorgeous-ways-applaud.md b/.changeset/gorgeous-ways-applaud.md new file mode 100644 index 0000000000..0149061f0f --- /dev/null +++ b/.changeset/gorgeous-ways-applaud.md @@ -0,0 +1,8 @@ +--- +'@backstage/plugin-signals-backend': patch +'@backstage/plugin-signals-react': patch +'@backstage/plugin-signals-node': patch +'@backstage/plugin-signals': patch +--- + +Fix to disconnect loop on server start diff --git a/packages/backend/src/plugins/signals.ts b/packages/backend/src/plugins/signals.ts index 477cea1938..066fae74f8 100644 --- a/packages/backend/src/plugins/signals.ts +++ b/packages/backend/src/plugins/signals.ts @@ -24,5 +24,6 @@ export default async function createPlugin( logger: env.logger, eventBroker: env.eventBroker, identity: env.identity, + discovery: env.discovery, }); } diff --git a/plugins/signals-backend/README.md b/plugins/signals-backend/README.md index e30a0836e0..ff1fcb6562 100644 --- a/plugins/signals-backend/README.md +++ b/plugins/signals-backend/README.md @@ -22,6 +22,7 @@ export default async function createPlugin( logger: env.logger, eventBroker: env.eventBroker, identity: env.identity, + discovery: env.discovery, }); } ``` diff --git a/plugins/signals-backend/api-report.md b/plugins/signals-backend/api-report.md index dd46778e5d..8bdaa581f3 100644 --- a/plugins/signals-backend/api-report.md +++ b/plugins/signals-backend/api-report.md @@ -8,12 +8,15 @@ import { EventBroker } from '@backstage/plugin-events-node'; import express from 'express'; import { IdentityApi } from '@backstage/plugin-auth-node'; import { LoggerService } from '@backstage/backend-plugin-api'; +import { PluginEndpointDiscovery } from '@backstage/backend-common'; // @public (undocumented) export function createRouter(options: RouterOptions): Promise; // @public (undocumented) export interface RouterOptions { + // (undocumented) + discovery: PluginEndpointDiscovery; // (undocumented) eventBroker?: EventBroker; // (undocumented) diff --git a/plugins/signals-backend/src/plugin.ts b/plugins/signals-backend/src/plugin.ts index 4a9cef028f..11b63163d5 100644 --- a/plugins/signals-backend/src/plugin.ts +++ b/plugins/signals-backend/src/plugin.ts @@ -32,14 +32,16 @@ export const signalsPlugin = createBackendPlugin({ httpRouter: coreServices.httpRouter, logger: coreServices.logger, identity: coreServices.identity, + discovery: coreServices.discovery, // TODO: EventBroker. It is optional for now but it's actually required so waiting for the new backend system // for the events-backend for this to work. }, - async init({ httpRouter, logger, identity }) { + async init({ httpRouter, logger, identity, discovery }) { httpRouter.use( await createRouter({ logger, identity, + discovery, }), ); }, diff --git a/plugins/signals-backend/src/service/SignalManager.test.ts b/plugins/signals-backend/src/service/SignalManager.test.ts index 5720d1ff52..76cff0d44e 100644 --- a/plugins/signals-backend/src/service/SignalManager.test.ts +++ b/plugins/signals-backend/src/service/SignalManager.test.ts @@ -89,7 +89,7 @@ describe('SignalManager', () => { await onEvent({ topic: 'signals', eventPayload: { - recipients: null, + receivers: null, channel: 'test', message: { msg: 'test' }, }, @@ -109,7 +109,7 @@ describe('SignalManager', () => { await onEvent({ topic: 'signals', eventPayload: { - recipients: null, + receivers: null, channel: 'test', message: { msg: 'test' }, }, @@ -162,7 +162,7 @@ describe('SignalManager', () => { await onEvent({ topic: 'signals', eventPayload: { - recipients: 'user:default/john.doe', + receivers: 'user:default/john.doe', channel: 'test', message: { msg: 'test' }, }, diff --git a/plugins/signals-backend/src/service/SignalManager.ts b/plugins/signals-backend/src/service/SignalManager.ts index 983496b96b..1f1681b86c 100644 --- a/plugins/signals-backend/src/service/SignalManager.ts +++ b/plugins/signals-backend/src/service/SignalManager.ts @@ -71,7 +71,9 @@ export class SignalManager { id, user: identity?.identity.userEntityRef ?? 'user:default/guest', ws, - ownershipEntityRefs: identity?.identity.ownershipEntityRefs ?? [], + ownershipEntityRefs: identity?.identity.ownershipEntityRefs ?? [ + 'user:default/guest', + ], subscriptions: new Set(), }; @@ -130,8 +132,12 @@ export class SignalManager { return; } - const { channel, recipients, message } = eventPayload; + const { channel, receivers, message } = eventPayload; const jsonMessage = JSON.stringify({ channel, message }); + let users: string[] = []; + if (receivers !== null) { + users = Array.isArray(receivers) ? receivers : [receivers]; + } // Actual websocket message sending this.connections.forEach(conn => { @@ -140,10 +146,8 @@ export class SignalManager { } // Sending to all users can be done with null if ( - recipients !== null && - !conn.ownershipEntityRefs.some((ref: string) => - recipients.includes(ref), - ) + receivers !== null && + !conn.ownershipEntityRefs.some((ref: string) => users.includes(ref)) ) { return; } diff --git a/plugins/signals-backend/src/service/router.test.ts b/plugins/signals-backend/src/service/router.test.ts index ecf2ad4aa6..807b45d53e 100644 --- a/plugins/signals-backend/src/service/router.test.ts +++ b/plugins/signals-backend/src/service/router.test.ts @@ -13,7 +13,10 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { getVoidLogger } from '@backstage/backend-common'; +import { + getVoidLogger, + PluginEndpointDiscovery, +} from '@backstage/backend-common'; import express from 'express'; import request from 'supertest'; @@ -30,6 +33,11 @@ const identityApiMock: jest.Mocked = { getIdentity: jest.fn(), }; +const discovery: jest.Mocked = { + getBaseUrl: jest.fn().mockResolvedValue('/api/signals'), + getExternalBaseUrl: jest.fn(), +}; + describe('createRouter', () => { let app: express.Express; @@ -38,6 +46,7 @@ describe('createRouter', () => { logger: getVoidLogger(), identity: identityApiMock, eventBroker: eventBrokerMock, + discovery, }); app = express().use(router); }); diff --git a/plugins/signals-backend/src/service/router.ts b/plugins/signals-backend/src/service/router.ts index 4394a9f480..cc8fe9b851 100644 --- a/plugins/signals-backend/src/service/router.ts +++ b/plugins/signals-backend/src/service/router.ts @@ -13,7 +13,10 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { errorHandler } from '@backstage/backend-common'; +import { + errorHandler, + PluginEndpointDiscovery, +} from '@backstage/backend-common'; import express, { NextFunction, Request, Response } from 'express'; import Router from 'express-promise-router'; import { LoggerService } from '@backstage/backend-plugin-api'; @@ -33,13 +36,14 @@ export interface RouterOptions { logger: LoggerService; eventBroker?: EventBroker; identity: IdentityApi; + discovery: PluginEndpointDiscovery; } /** @public */ export async function createRouter( options: RouterOptions, ): Promise { - const { logger, identity } = options; + const { logger, identity, discovery } = options; const manager = SignalManager.create(options); let subscribedToUpgradeRequests = false; @@ -66,9 +70,9 @@ export async function createRouter( } subscribedToUpgradeRequests = true; + const apiUrl = await discovery.getBaseUrl('signals'); server.on('upgrade', async (request, socket, head) => { - // TODO: Find a way to make this more generic - if (request.url !== '/api/signals') { + if (!request.url || !apiUrl.endsWith(request.url)) { return; } diff --git a/plugins/signals-backend/src/service/standaloneServer.ts b/plugins/signals-backend/src/service/standaloneServer.ts index 278de8621e..311d3f8587 100644 --- a/plugins/signals-backend/src/service/standaloneServer.ts +++ b/plugins/signals-backend/src/service/standaloneServer.ts @@ -68,6 +68,7 @@ export async function startStandaloneServer( logger, identity, eventBroker, + discovery, }); let service = createServiceBuilder(module) @@ -83,7 +84,7 @@ export async function startStandaloneServer( setInterval(() => { signals.publish({ - recipients: null, + receivers: null, channel: 'test', message: { hello: 'world' }, }); diff --git a/plugins/signals-node/api-report.md b/plugins/signals-node/api-report.md index 00d099f7db..bfd8543a17 100644 --- a/plugins/signals-node/api-report.md +++ b/plugins/signals-node/api-report.md @@ -16,15 +16,15 @@ export class DefaultSignalService implements SignalService { // @public (undocumented) export type SignalPayload = { - recipients: string[] | null; + receivers: string[] | string | null; channel: string; message: JsonObject; }; // @public (undocumented) -export type SignalService = { +export interface SignalService { publish(signal: SignalPayload): Promise; -}; +} // @public (undocumented) export const signalService: ServiceRef; diff --git a/plugins/signals-node/src/DefaultSignalService.ts b/plugins/signals-node/src/DefaultSignalService.ts index 1fba96b8bc..4824c1219f 100644 --- a/plugins/signals-node/src/DefaultSignalService.ts +++ b/plugins/signals-node/src/DefaultSignalService.ts @@ -37,11 +37,11 @@ export class DefaultSignalService implements SignalService { * @param message - message to publish */ async publish(signal: SignalPayload) { - const { recipients, channel, message } = signal; + const { receivers, channel, message } = signal; await this.eventBroker?.publish({ topic: 'signals', eventPayload: { - recipients, + receivers, message, channel, }, diff --git a/plugins/signals-node/src/SignalService.ts b/plugins/signals-node/src/SignalService.ts index f08a12661f..7d021ccf4e 100644 --- a/plugins/signals-node/src/SignalService.ts +++ b/plugins/signals-node/src/SignalService.ts @@ -16,9 +16,9 @@ import { SignalPayload } from './types'; /** @public */ -export type SignalService = { +export interface SignalService { /** * Publishes a message to user refs to specific topic */ publish(signal: SignalPayload): Promise; -}; +} diff --git a/plugins/signals-node/src/types.ts b/plugins/signals-node/src/types.ts index 0220a196aa..21af773268 100644 --- a/plugins/signals-node/src/types.ts +++ b/plugins/signals-node/src/types.ts @@ -25,7 +25,7 @@ export type SignalServiceOptions = { /** @public */ export type SignalPayload = { - recipients: string[] | null; + receivers: string[] | string | null; channel: string; message: JsonObject; }; diff --git a/plugins/signals-react/api-report.md b/plugins/signals-react/api-report.md index 2df2e4f1ce..e3c4f740a5 100644 --- a/plugins/signals-react/api-report.md +++ b/plugins/signals-react/api-report.md @@ -7,18 +7,23 @@ import { ApiRef } from '@backstage/core-plugin-api'; import { JsonObject } from '@backstage/types'; // @public (undocumented) -export type SignalApi = { +export interface SignalApi { + // (undocumented) subscribe( channel: string, onMessage: (message: JsonObject) => void, - ): { - unsubscribe: () => void; - }; -}; + ): SignalSubscriber; +} // @public (undocumented) export const signalApiRef: ApiRef; +// @public (undocumented) +export interface SignalSubscriber { + // (undocumented) + unsubscribe(): void; +} + // @public (undocumented) export const useSignal: (channel: string) => { lastSignal: JsonObject | null; diff --git a/plugins/signals-react/src/api/SignalApi.ts b/plugins/signals-react/src/api/SignalApi.ts index b37b3ae2f5..b67b2ea0dc 100644 --- a/plugins/signals-react/src/api/SignalApi.ts +++ b/plugins/signals-react/src/api/SignalApi.ts @@ -22,9 +22,14 @@ export const signalApiRef = createApiRef({ }); /** @public */ -export type SignalApi = { +export interface SignalSubscriber { + unsubscribe(): void; +} + +/** @public */ +export interface SignalApi { subscribe( channel: string, onMessage: (message: JsonObject) => void, - ): { unsubscribe: () => void }; -}; + ): SignalSubscriber; +} diff --git a/plugins/signals/src/api/SignalClient.ts b/plugins/signals/src/api/SignalClient.ts index ee4ed8756b..d6c4634264 100644 --- a/plugins/signals/src/api/SignalClient.ts +++ b/plugins/signals/src/api/SignalClient.ts @@ -146,20 +146,6 @@ export class SignalClient implements SignalApi { url.protocol = url.protocol === 'http:' ? 'ws:' : 'wss:'; this.ws = new WebSocket(url.toString(), token); - this.ws.onmessage = (data: MessageEvent) => { - this.handleMessage(data); - }; - - this.ws.onerror = () => { - this.reconnect(); - }; - - this.ws.onclose = (ev: CloseEvent) => { - if (ev.code !== WS_CLOSE_NORMAL && ev.code !== WS_CLOSE_GOING_AWAY) { - this.reconnect(); - } - }; - // Wait until connection is open let connectSleep = 0; while ( @@ -174,6 +160,20 @@ export class SignalClient implements SignalApi { if (!this.ws || this.ws.readyState !== WebSocket.OPEN) { throw new Error('Connect timeout'); } + + this.ws.onmessage = (data: MessageEvent) => { + this.handleMessage(data); + }; + + this.ws.onerror = () => { + this.reconnect(); + }; + + this.ws.onclose = (ev: CloseEvent) => { + if (ev.code !== WS_CLOSE_NORMAL && ev.code !== WS_CLOSE_GOING_AWAY) { + this.reconnect(); + } + }; } private handleMessage(data: MessageEvent) {