From 447d21045b791e1e2dca38bc9fd3ee169e25e1a3 Mon Sep 17 00:00:00 2001 From: Heikki Hellgren Date: Fri, 26 Jan 2024 09:31:06 +0200 Subject: [PATCH 1/3] 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) { From dde856b22847876302a2ac478659db50faa653f5 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Thu, 1 Feb 2024 17:07:49 +0100 Subject: [PATCH 2/3] Update .changeset/gorgeous-ways-applaud.md Signed-off-by: Patrik Oldsberg --- .changeset/gorgeous-ways-applaud.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/gorgeous-ways-applaud.md b/.changeset/gorgeous-ways-applaud.md index 0149061f0f..4b28e2e656 100644 --- a/.changeset/gorgeous-ways-applaud.md +++ b/.changeset/gorgeous-ways-applaud.md @@ -5,4 +5,4 @@ '@backstage/plugin-signals': patch --- -Fix to disconnect loop on server start +Fix disconnect loop on server start From 7ce5f0009a058e20f5bb330976f3325084ea0d1b Mon Sep 17 00:00:00 2001 From: Heikki Hellgren Date: Thu, 1 Feb 2024 20:28:04 +0200 Subject: [PATCH 3/3] chore: restore back to recipients + add test Signed-off-by: Heikki Hellgren --- .../src/service/SignalManager.test.ts | 6 +-- .../src/service/SignalManager.ts | 8 ++-- .../src/service/standaloneServer.ts | 2 +- plugins/signals-node/api-report.md | 2 +- .../src/DefaultSignalService.test.ts | 38 +++++++++++++++++++ .../signals-node/src/DefaultSignalService.ts | 7 +--- plugins/signals-node/src/types.ts | 2 +- plugins/signals-react/api-report.md | 1 + plugins/signals-react/src/hooks/useSignal.ts | 7 +++- 9 files changed, 55 insertions(+), 18 deletions(-) create mode 100644 plugins/signals-node/src/DefaultSignalService.test.ts diff --git a/plugins/signals-backend/src/service/SignalManager.test.ts b/plugins/signals-backend/src/service/SignalManager.test.ts index 76cff0d44e..5720d1ff52 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: { - receivers: null, + recipients: null, channel: 'test', message: { msg: 'test' }, }, @@ -109,7 +109,7 @@ describe('SignalManager', () => { await onEvent({ topic: 'signals', eventPayload: { - receivers: null, + recipients: null, channel: 'test', message: { msg: 'test' }, }, @@ -162,7 +162,7 @@ describe('SignalManager', () => { await onEvent({ topic: 'signals', eventPayload: { - receivers: 'user:default/john.doe', + recipients: '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 1f1681b86c..db374418e1 100644 --- a/plugins/signals-backend/src/service/SignalManager.ts +++ b/plugins/signals-backend/src/service/SignalManager.ts @@ -132,11 +132,11 @@ export class SignalManager { return; } - const { channel, receivers, message } = eventPayload; + const { channel, recipients, message } = eventPayload; const jsonMessage = JSON.stringify({ channel, message }); let users: string[] = []; - if (receivers !== null) { - users = Array.isArray(receivers) ? receivers : [receivers]; + if (recipients !== null) { + users = Array.isArray(recipients) ? recipients : [recipients]; } // Actual websocket message sending @@ -146,7 +146,7 @@ export class SignalManager { } // Sending to all users can be done with null if ( - receivers !== null && + recipients !== null && !conn.ownershipEntityRefs.some((ref: string) => users.includes(ref)) ) { return; diff --git a/plugins/signals-backend/src/service/standaloneServer.ts b/plugins/signals-backend/src/service/standaloneServer.ts index 311d3f8587..00ef7b5c77 100644 --- a/plugins/signals-backend/src/service/standaloneServer.ts +++ b/plugins/signals-backend/src/service/standaloneServer.ts @@ -84,7 +84,7 @@ export async function startStandaloneServer( setInterval(() => { signals.publish({ - receivers: null, + recipients: null, channel: 'test', message: { hello: 'world' }, }); diff --git a/plugins/signals-node/api-report.md b/plugins/signals-node/api-report.md index bfd8543a17..cbf4142daf 100644 --- a/plugins/signals-node/api-report.md +++ b/plugins/signals-node/api-report.md @@ -16,7 +16,7 @@ export class DefaultSignalService implements SignalService { // @public (undocumented) export type SignalPayload = { - receivers: string[] | string | null; + recipients: string[] | string | null; channel: string; message: JsonObject; }; diff --git a/plugins/signals-node/src/DefaultSignalService.test.ts b/plugins/signals-node/src/DefaultSignalService.test.ts new file mode 100644 index 0000000000..9938c84c86 --- /dev/null +++ b/plugins/signals-node/src/DefaultSignalService.test.ts @@ -0,0 +1,38 @@ +/* + * Copyright 2024 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { DefaultSignalService } from './DefaultSignalService'; + +describe('DefaultSignalService', () => { + const mockEventBroker = { + publish: jest.fn(), + subscribe: jest.fn(), + }; + + const service = DefaultSignalService.create({ eventBroker: mockEventBroker }); + + it('should publish signal', () => { + const signal = { + channel: 'test-channel', + recipients: null, + message: { msg: 'hello world' }, + }; + service.publish(signal); + expect(mockEventBroker.publish).toHaveBeenCalledWith({ + topic: 'signals', + eventPayload: signal, + }); + }); +}); diff --git a/plugins/signals-node/src/DefaultSignalService.ts b/plugins/signals-node/src/DefaultSignalService.ts index 4824c1219f..2a234fe5d4 100644 --- a/plugins/signals-node/src/DefaultSignalService.ts +++ b/plugins/signals-node/src/DefaultSignalService.ts @@ -37,14 +37,9 @@ export class DefaultSignalService implements SignalService { * @param message - message to publish */ async publish(signal: SignalPayload) { - const { receivers, channel, message } = signal; await this.eventBroker?.publish({ topic: 'signals', - eventPayload: { - receivers, - message, - channel, - }, + eventPayload: signal, }); } } diff --git a/plugins/signals-node/src/types.ts b/plugins/signals-node/src/types.ts index 21af773268..7e61aea320 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 = { - receivers: string[] | string | null; + recipients: string[] | string | null; channel: string; message: JsonObject; }; diff --git a/plugins/signals-react/api-report.md b/plugins/signals-react/api-report.md index e3c4f740a5..ef4bc1b7fc 100644 --- a/plugins/signals-react/api-report.md +++ b/plugins/signals-react/api-report.md @@ -27,6 +27,7 @@ export interface SignalSubscriber { // @public (undocumented) export const useSignal: (channel: string) => { lastSignal: JsonObject | null; + isSignalsAvailable: boolean; }; // (No @packageDocumentation comment for this package) diff --git a/plugins/signals-react/src/hooks/useSignal.ts b/plugins/signals-react/src/hooks/useSignal.ts index a7d50fbe3b..084427e2cc 100644 --- a/plugins/signals-react/src/hooks/useSignal.ts +++ b/plugins/signals-react/src/hooks/useSignal.ts @@ -16,7 +16,7 @@ import { signalApiRef } from '../api'; import { useApiHolder } from '@backstage/core-plugin-api'; import { JsonObject } from '@backstage/types'; -import { useEffect, useState } from 'react'; +import { useEffect, useMemo, useState } from 'react'; /** @public */ export const useSignal = (channel: string) => { @@ -40,5 +40,8 @@ export const useSignal = (channel: string) => { }; }, [signals, channel]); - return { lastSignal }; + // Can be used to fallback (for example to long polling) if signals are not available in the system + const isSignalsAvailable = useMemo(() => !signals, [signals]); + + return { lastSignal, isSignalsAvailable }; };