From b6faed0db3cb2b6282acdafb64e1225c89e66842 Mon Sep 17 00:00:00 2001 From: zg009 <103973669+zg009@users.noreply.github.com> Date: Mon, 13 Mar 2023 02:30:42 -0500 Subject: [PATCH] fix: Updated WrappedExpiringStorage to use timer.unref * fix: updated WrappedExpiringStorage tests and timer.unref calls * fix: removed finalizable configs and inheritors that only used timer * fix: updated test function to test setSafeInterval and timer.unref --- .../handler/account-store/default.json | 11 ------ config/identity/ownership/token.json | 11 ------ .../readers/access-checkers/agent-group.json | 11 ------ .../keyvalue/WrappedExpiringStorage.ts | 11 ++---- .../keyvalue/WrappedExpiringStorage.test.ts | 36 ++++++------------- 5 files changed, 12 insertions(+), 68 deletions(-) diff --git a/config/identity/handler/account-store/default.json b/config/identity/handler/account-store/default.json index c547e117e..68ddc0db2 100644 --- a/config/identity/handler/account-store/default.json +++ b/config/identity/handler/account-store/default.json @@ -24,17 +24,6 @@ "relativePath": "/forgot-password/", "source": { "@id": "urn:solid-server:default:KeyValueStorage" } } - }, - { - "comment": "Makes sure the expiring storage cleanup timer is stopped when the application needs to stop.", - "@id": "urn:solid-server:default:Finalizer", - "@type": "ParallelHandler", - "handlers": [ - { - "@type": "FinalizableHandler", - "finalizable": { "@id": "urn:solid-server:default:ExpiringForgotPasswordStorage" } - } - ] } ] } diff --git a/config/identity/ownership/token.json b/config/identity/ownership/token.json index 551d33467..b24921687 100644 --- a/config/identity/ownership/token.json +++ b/config/identity/ownership/token.json @@ -17,17 +17,6 @@ "relativePath": "/idp/tokens/", "source": { "@id": "urn:solid-server:default:KeyValueStorage" } } - }, - { - "comment": "Makes sure the expiring storage cleanup timer is stopped when the application needs to stop.", - "@id": "urn:solid-server:default:Finalizer", - "@type": "ParallelHandler", - "handlers": [ - { - "@type": "FinalizableHandler", - "finalizable": { "@id": "urn:solid-server:default:ExpiringTokenStorage" } - } - ] } ] } diff --git a/config/ldp/authorization/readers/access-checkers/agent-group.json b/config/ldp/authorization/readers/access-checkers/agent-group.json index 994bd8e92..a5ee84fcf 100644 --- a/config/ldp/authorization/readers/access-checkers/agent-group.json +++ b/config/ldp/authorization/readers/access-checkers/agent-group.json @@ -10,17 +10,6 @@ "@type": "WrappedExpiringStorage", "source": { "@type": "MemoryMapStorage" } } - }, - { - "comment": "Makes sure the expiring storage cleanup timer is stopped when the application needs to stop.", - "@id": "urn:solid-server:default:Finalizer", - "@type": "ParallelHandler", - "handlers": [ - { - "@type": "FinalizableHandler", - "finalizable": { "@id": "urn:solid-server:default:ExpiringAclCache" } - } - ] } ] } diff --git a/src/storage/keyvalue/WrappedExpiringStorage.ts b/src/storage/keyvalue/WrappedExpiringStorage.ts index d94196a2f..32a9f062c 100644 --- a/src/storage/keyvalue/WrappedExpiringStorage.ts +++ b/src/storage/keyvalue/WrappedExpiringStorage.ts @@ -1,4 +1,3 @@ -import type { Finalizable } from '../../init/final/Finalizable'; import { getLoggerFor } from '../../logging/LogUtil'; import { InternalServerError } from '../../util/errors/InternalServerError'; import { setSafeInterval } from '../../util/TimerUtil'; @@ -13,7 +12,7 @@ export type Expires = { expires?: string; payload: T }; * Will delete expired entries when trying to get their value. * Has a timer that will delete all expired data every hour (default value). */ -export class WrappedExpiringStorage implements ExpiringStorage, Finalizable { +export class WrappedExpiringStorage implements ExpiringStorage { protected readonly logger = getLoggerFor(this); private readonly source: KeyValueStorage>; private readonly timer: NodeJS.Timeout; @@ -28,6 +27,7 @@ export class WrappedExpiringStorage implements ExpiringStorage { @@ -121,11 +121,4 @@ export class WrappedExpiringStorage implements ExpiringStorage { - clearInterval(this.timer); - } } diff --git a/test/unit/storage/keyvalue/WrappedExpiringStorage.test.ts b/test/unit/storage/keyvalue/WrappedExpiringStorage.test.ts index a1af5ef88..9cad6b134 100644 --- a/test/unit/storage/keyvalue/WrappedExpiringStorage.test.ts +++ b/test/unit/storage/keyvalue/WrappedExpiringStorage.test.ts @@ -121,7 +121,12 @@ describe('A WrappedExpiringStorage', (): void => { // Disable interval function and simply check it was called with the correct parameters // Otherwise it gets quite difficult to verify the async interval function gets executed const mockInterval = jest.spyOn(global, 'setInterval'); - mockInterval.mockImplementation(jest.fn()); + + // We only need to call the timer.unref() once when the object is created + const mockTimer = { unref: jest.fn() }; + const mockFn = jest.fn().mockReturnValueOnce(mockTimer); + mockInterval.mockImplementationOnce(mockFn); + // Timeout of 1 minute storage = new WrappedExpiringStorage(source, 1); const data = [ @@ -141,33 +146,12 @@ describe('A WrappedExpiringStorage', (): void => { // Await the function that should have been executed by the interval await (mockInterval.mock.calls[0][0] as () => Promise)(); + // Make sure timer.unref() is called on initialization + expect(mockTimer.unref).toHaveBeenCalledTimes(1); + // Make sure setSafeInterval has been called once as well + expect(mockFn).toHaveBeenCalledTimes(1); expect(source.delete).toHaveBeenCalledTimes(1); expect(source.delete).toHaveBeenLastCalledWith('key2'); mockInterval.mockRestore(); }); - - it('can stop the timer.', async(): Promise => { - const mockInterval = jest.spyOn(global, 'setInterval'); - const mockClear = jest.spyOn(global, 'clearInterval'); - // Timeout of 1 minute - storage = new WrappedExpiringStorage(source, 1); - const data = [ - [ 'key1', createExpires('data1', tomorrow) ], - [ 'key2', createExpires('data2', yesterday) ], - [ 'key3', createExpires('data3') ], - ]; - source.entries.mockImplementationOnce(function* (): any { - yield* data; - }); - - await expect(storage.finalize()).resolves.toBeUndefined(); - - // Make sure clearInterval was called with the interval timer - expect(mockClear.mock.calls).toHaveLength(1); - expect(mockClear.mock.calls[0]).toHaveLength(1); - expect(mockClear.mock.calls[0][0]).toBe(mockInterval.mock.results[0].value); - - mockInterval.mockRestore(); - mockClear.mockRestore(); - }); });