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

chore: improve resource disposal #1175

Merged
merged 3 commits into from
Jun 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 12 additions & 12 deletions packages/backend/src/managers/applicationManager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1001,7 +1001,7 @@ describe('pod detection', async () => {
);
});

test('adoptRunningApplications updates the app state with the found pod', async () => {
test('init updates the app state with the found pod', async () => {
vi.mocked(podManager.getPodsWithLabels).mockResolvedValue([
{
Labels: {
Expand All @@ -1016,7 +1016,7 @@ describe('pod detection', async () => {
f();
});
const updateApplicationStateSpy = vi.spyOn(manager, 'updateApplicationState');
manager.adoptRunningApplications();
manager.init();
await new Promise(resolve => setTimeout(resolve, 0));
expect(updateApplicationStateSpy).toHaveBeenNthCalledWith(1, 'recipe-id-1', 'model-id-1', {
pod: {
Expand All @@ -1037,13 +1037,13 @@ describe('pod detection', async () => {
expect(ports).toStrictEqual([5000, 5001]);
});

test('adoptRunningApplications does not update the application state with the found pod without label', async () => {
test('init does not update the application state with the found pod without label', async () => {
vi.mocked(podManager.getPodsWithLabels).mockResolvedValue([{} as unknown as PodInfo]);
mocks.startupSubscribeMock.mockImplementation((f: startupHandle) => {
f();
});
const updateApplicationStateSpy = vi.spyOn(manager, 'updateApplicationState');
manager.adoptRunningApplications();
manager.init();
await new Promise(resolve => setTimeout(resolve, 0));
expect(updateApplicationStateSpy).not.toHaveBeenCalled();
});
Expand All @@ -1054,7 +1054,7 @@ describe('pod detection', async () => {
f();
});
const sendApplicationStateSpy = vi.spyOn(manager, 'notify').mockResolvedValue();
manager.adoptRunningApplications();
manager.init();
expect(sendApplicationStateSpy).toHaveBeenCalledOnce();
});

Expand All @@ -1074,7 +1074,7 @@ describe('pod detection', async () => {
return { dispose: vi.fn() };
});
const sendApplicationStateSpy = vi.spyOn(manager, 'notify').mockResolvedValue();
manager.adoptRunningApplications();
manager.init();
expect(sendApplicationStateSpy).toHaveBeenCalledOnce();
});

Expand All @@ -1090,7 +1090,7 @@ describe('pod detection', async () => {
return { dispose: vi.fn() };
});
const sendApplicationStateSpy = vi.spyOn(manager, 'notify').mockResolvedValue();
manager.adoptRunningApplications();
manager.init();
expect(sendApplicationStateSpy).not.toHaveBeenCalledOnce();
});

Expand All @@ -1109,7 +1109,7 @@ describe('pod detection', async () => {
return { dispose: vi.fn() };
});
const sendApplicationStateSpy = vi.spyOn(manager, 'notify').mockResolvedValue();
manager.adoptRunningApplications();
manager.init();
expect(sendApplicationStateSpy).not.toHaveBeenCalledOnce();
});

Expand Down Expand Up @@ -1142,7 +1142,7 @@ describe('pod detection', async () => {
return { dispose: vi.fn() };
});
const sendApplicationStateSpy = vi.spyOn(manager, 'notify').mockResolvedValue();
manager.adoptRunningApplications();
manager.init();
await new Promise(resolve => setTimeout(resolve, 10));
expect(sendApplicationStateSpy).toHaveBeenCalledTimes(1);
});
Expand Down Expand Up @@ -1170,7 +1170,7 @@ describe('pod detection', async () => {
return { dispose: vi.fn() };
});
const sendApplicationStateSpy = vi.spyOn(manager, 'notify').mockResolvedValue();
manager.adoptRunningApplications();
manager.init();
await new Promise(resolve => setTimeout(resolve, 10));
expect(sendApplicationStateSpy).toHaveBeenCalledTimes(2);
});
Expand Down Expand Up @@ -1226,7 +1226,7 @@ describe('pod detection', async () => {
expect(podManager.removePod).toHaveBeenCalledWith('engine-1', 'pod-1');
});

test('adoptRunningApplications should check pods health', async () => {
test('init should check pods health', async () => {
vi.mocked(podManager.getHealth).mockResolvedValue('healthy');
vi.mocked(podManager.getPodsWithLabels).mockResolvedValue([
{
Expand Down Expand Up @@ -1255,7 +1255,7 @@ describe('pod detection', async () => {
f();
});
vi.useFakeTimers();
manager.adoptRunningApplications();
manager.init();
await vi.advanceTimersByTimeAsync(1100);
const state = manager.getApplicationsState();
expect(state).toHaveLength(1);
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/managers/applicationManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,7 @@ export class ApplicationManager extends Publisher<ApplicationState[]> implements
}
}

adoptRunningApplications() {
init() {
this.podmanConnection.startupSubscribe(() => {
this.podManager
.getPodsWithLabels([LABEL_RECIPE_ID])
Expand Down
8 changes: 4 additions & 4 deletions packages/backend/src/managers/modelsManager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -456,9 +456,9 @@ test('deleteModel deletes the model folder', async () => {
maxRetries: 3,
});
}
expect(postMessageMock).toHaveBeenCalledTimes(3);
expect(postMessageMock).toHaveBeenCalledTimes(4);
// check that a new state is sent with the model removed
expect(postMessageMock).toHaveBeenNthCalledWith(3, {
expect(postMessageMock).toHaveBeenNthCalledWith(4, {
id: 'new-models-state',
body: [
{
Expand Down Expand Up @@ -522,9 +522,9 @@ describe('deleting models', () => {
maxRetries: 3,
});
}
expect(postMessageMock).toHaveBeenCalledTimes(3);
expect(postMessageMock).toHaveBeenCalledTimes(4);
// check that a new state is sent with the model non removed
expect(postMessageMock).toHaveBeenNthCalledWith(3, {
expect(postMessageMock).toHaveBeenNthCalledWith(4, {
id: 'new-models-state',
body: [
{
Expand Down
4 changes: 4 additions & 0 deletions packages/backend/src/managers/modelsManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ export class ModelsManager implements Disposable {
});
});
this.#disposables.push(disposable);

this.loadLocalModels().catch((err: unknown) => {
console.error('Something went wrong while trying to load local models', err);
});
}

dispose(): void {
Expand Down
12 changes: 9 additions & 3 deletions packages/backend/src/registries/ContainerRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,17 @@ export interface ContainerStart {
id: string;
}

export class ContainerRegistry {
export class ContainerRegistry implements podmanDesktopApi.Disposable {
private count: number = 0;
private subscribers: Map<string, Subscriber[]> = new Map();

private readonly _onStartContainerEvent = new podmanDesktopApi.EventEmitter<ContainerStart>();
readonly onStartContainerEvent: podmanDesktopApi.Event<ContainerStart> = this._onStartContainerEvent.event;

init(): podmanDesktopApi.Disposable {
return podmanDesktopApi.containerEngine.onEvent(event => {
#eventDisposable: podmanDesktopApi.Disposable | undefined;

init(): void {
this.#eventDisposable = podmanDesktopApi.containerEngine.onEvent(event => {
if (event.status === 'start') {
this._onStartContainerEvent.fire({
id: event.id,
Expand All @@ -52,6 +54,10 @@ export class ContainerRegistry {
});
}

dispose(): void {
this.#eventDisposable?.dispose();
}

subscribe(containerId: string, callback: (status: string) => void): podmanDesktopApi.Disposable {
const subscriberId = ++this.count;
const nSubs: Subscriber[] = [
Expand Down
45 changes: 34 additions & 11 deletions packages/backend/src/registries/LocalRepositoryRegistry.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import type { Webview } from '@podman-desktop/api';
import type { Recipe } from '@shared/src/models/IRecipe';
import fs from 'node:fs';
import path from 'node:path';
import type { CatalogManager } from '../managers/catalogManager';

const mocks = vi.hoisted(() => ({
DisposableCreateMock: vi.fn(),
Expand All @@ -45,6 +46,11 @@ vi.mock('node:fs', () => {
};
});

const catalogManagerMock = {
onCatalogUpdate: vi.fn(),
getRecipes: vi.fn(),
} as unknown as CatalogManager;

beforeEach(() => {
vi.resetAllMocks();
vi.mock('node:fs');
Expand All @@ -57,6 +63,7 @@ test('should not have any repositories by default', () => {
postMessage: mocks.postMessageMock,
} as unknown as Webview,
'/appUserDirectory',
catalogManagerMock,
);
expect(localRepositories.getLocalRepositories().length).toBe(0);
});
Expand All @@ -67,6 +74,7 @@ test('should notify webview when register', () => {
postMessage: mocks.postMessageMock,
} as unknown as Webview,
'/appUserDirectory',
catalogManagerMock,
);
localRepositories.register({ path: 'random', sourcePath: 'random', labels: { 'recipe-id': 'random' } });
expect(mocks.postMessageMock).toHaveBeenNthCalledWith(1, {
Expand All @@ -81,6 +89,7 @@ test('should notify webview when unregister', async () => {
postMessage: mocks.postMessageMock,
} as unknown as Webview,
'/appUserDirectory',
catalogManagerMock,
);
vi.spyOn(fs.promises, 'rm').mockResolvedValue();
localRepositories.register({ path: 'random', sourcePath: 'random', labels: { 'recipe-id': 'random' } });
Expand All @@ -94,18 +103,26 @@ test('should notify webview when unregister', async () => {

test('should register localRepo if it find the folder of the recipe', () => {
vi.spyOn(fs, 'existsSync').mockReturnValue(true);
vi.mocked(catalogManagerMock.onCatalogUpdate).mockImplementation((fn: () => void) => {
fn();
return { dispose: vi.fn() };
});
vi.mocked(catalogManagerMock.getRecipes).mockReturnValue([
{
id: 'recipe',
} as unknown as Recipe,
]);

const localRepositories = new LocalRepositoryRegistry(
{
postMessage: mocks.postMessageMock,
} as unknown as Webview,
'/appUserDirectory',
catalogManagerMock,
);

const registerMock = vi.spyOn(localRepositories, 'register');
localRepositories.init([
{
id: 'recipe',
} as unknown as Recipe,
]);
localRepositories.init();

const folder = path.join('/appUserDirectory', 'recipe');
expect(registerMock).toHaveBeenCalledWith({
Expand All @@ -117,18 +134,24 @@ test('should register localRepo if it find the folder of the recipe', () => {

test('should NOT register localRepo if it does not find the folder of the recipe', () => {
vi.spyOn(fs, 'existsSync').mockReturnValue(false);
vi.mocked(catalogManagerMock.onCatalogUpdate).mockImplementation((fn: () => void) => {
fn();
return { dispose: vi.fn() };
});
vi.mocked(catalogManagerMock.getRecipes).mockReturnValue([
{
id: 'recipe',
} as unknown as Recipe,
]);

const localRepositories = new LocalRepositoryRegistry(
{
postMessage: mocks.postMessageMock,
} as unknown as Webview,
'/appUserDirectory',
catalogManagerMock,
);
const registerMock = vi.spyOn(localRepositories, 'register');
localRepositories.init([
{
id: 'recipe',
} as unknown as Recipe,
]);

localRepositories.init();
expect(registerMock).not.toHaveBeenCalled();
});
15 changes: 12 additions & 3 deletions packages/backend/src/registries/LocalRepositoryRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,23 +22,32 @@ import { Publisher } from '../utils/Publisher';
import type { Recipe } from '@shared/src/models/IRecipe';
import fs from 'node:fs';
import path from 'node:path';
import type { CatalogManager } from '../managers/catalogManager';

/**
* The LocalRepositoryRegistry is responsible for keeping track of the directories where recipe are cloned
*/
export class LocalRepositoryRegistry extends Publisher<LocalRepository[]> {
export class LocalRepositoryRegistry extends Publisher<LocalRepository[]> implements Disposable {
// Map path => LocalRepository
private repositories: Map<string, LocalRepository> = new Map();
#catalogEventDisposable: Disposable | undefined;

constructor(
webview: Webview,
private appUserDirectory: string,
private catalogManager: CatalogManager,
) {
super(webview, Messages.MSG_LOCAL_REPOSITORY_UPDATE, () => this.getLocalRepositories());
}

init(recipes: Recipe[]): void {
this.loadLocalRecipeRepositories(recipes);
dispose(): void {
this.#catalogEventDisposable?.dispose();
}

init(): void {
this.#catalogEventDisposable = this.catalogManager.onCatalogUpdate(() => {
this.loadLocalRecipeRepositories(this.catalogManager.getRecipes());
});
}

register(localRepository: LocalRepository): Disposable {
Expand Down
1 change: 1 addition & 0 deletions packages/backend/src/studio-api-impl.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ beforeEach(async () => {
postMessage: vi.fn().mockResolvedValue(undefined),
} as unknown as Webview,
appUserDirectory,
{} as unknown as CatalogManager,
);

const telemetryMock = {
Expand Down
Loading