Skip to content

Fix DRACOLoader Web Worker leak during GLTF loading #795

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.

Sign up for GitHub

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

Open
WHOIM1205 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix DRACOLoader Web Worker leak during GLTF loading #795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
53 changes: 37 additions & 16 deletions packages/phoenix-event-display/src/managers/three-manager/import-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ export class ImportManager {
private EVENT_DATA_ID: string;
/** Object group ID containing detector geometries. */
private GEOMETRIES_ID: string;
/**
* Shared DRACOLoader instance for GLTF loading.
* Reused across all GLTF load operations to prevent Web Worker leaks.
* Must be disposed via cleanup() when the ImportManager is no longer needed.
*/
private dracoLoader: DRACOLoader | null = null;

/**
* Constructor for the import manager.
Expand All @@ -48,6 +54,33 @@ export class ImportManager {
this.GEOMETRIES_ID = GEOMETRIES_ID;
}

/**
* Get or create the shared DRACOLoader instance.
* Lazily initializes the loader on first use to avoid unnecessary resource allocation.
* @returns The shared DRACOLoader instance.
*/
private getDRACOLoader(): DRACOLoader {
if (!this.dracoLoader) {
this.dracoLoader = new DRACOLoader();
this.dracoLoader.setDecoderPath(
`https://cdn.jsdelivr.net/npm/three@0.${REVISION}.0/examples/jsm/libs/draco/`,
);
}
return this.dracoLoader;
}

/**
* Cleanup and dispose all resources held by the ImportManager.
* Must be called when the ImportManager is no longer needed to prevent memory leaks.
* Specifically disposes the shared DRACOLoader which holds Web Worker threads.
*/
public cleanup(): void {
if (this.dracoLoader) {
this.dracoLoader.dispose();
this.dracoLoader = null;
}
}

/**
* Loads an OBJ (.obj) geometry from the given filename.
* @param filename Path to the geometry.
Expand Down Expand Up @@ -182,12 +215,7 @@ export class ImportManager {
callback: (geometries?: Object3D, eventData?: Object3D) => void,
): Promise<void> {
const loader = new GLTFLoader();

const dracoLoader = new DRACOLoader();
dracoLoader.setDecoderPath(
`https://cdn.jsdelivr.net/npm/three@0.${REVISION}.0/examples/jsm/libs/draco/`,
);
loader.setDRACOLoader(dracoLoader);
loader.setDRACOLoader(this.getDRACOLoader());

const sceneString = JSON.stringify(scene, null, 2);

Expand Down Expand Up @@ -379,11 +407,7 @@ export class ImportManager {
initiallyVisible: boolean,
): Promise<GeometryUIParameters[]> {
const loader = new GLTFLoader();
const dracoLoader = new DRACOLoader();
dracoLoader.setDecoderPath(
`https://cdn.jsdelivr.net/npm/three@0.${REVISION}.0/examples/jsm/libs/draco/`,
);
loader.setDRACOLoader(dracoLoader);
loader.setDRACOLoader(this.getDRACOLoader());

return new Promise<GeometryUIParameters[]>((resolve, reject) => {
loader.parse(
Expand Down Expand Up @@ -491,11 +515,8 @@ export class ImportManager {
name: string,
): Promise<GeometryUIParameters[]> {
const loader = new GLTFLoader();
const dracoLoader = new DRACOLoader();
dracoLoader.setDecoderPath(
`https://cdn.jsdelivr.net/npm/three@0.${REVISION}.0/examples/jsm/libs/draco/`,
);
loader.setDRACOLoader(dracoLoader);
loader.setDRACOLoader(this.getDRACOLoader());

return new Promise<GeometryUIParameters[]>((resolve, reject) => {
loader.parse(
geometry,
Expand Down
3 changes: 3 additions & 0 deletions packages/phoenix-event-display/src/managers/three-manager/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1794,5 +1794,8 @@ export class ThreeManager {
if (this.selectionManager) {
this.selectionManager.cleanup();
}
if (this.importManager) {
this.importManager.cleanup();
}
}
}
202 changes: 202 additions & 0 deletions packages/phoenix-event-display/src/tests/managers/three-manager/import-manager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,202 @@
/**
* @jest-environment jsdom
*/
import { ImportManager } from '../../../managers/three-manager/import-manager';
import { Plane, Vector3 } from 'three';
import { DRACOLoader } from 'three/examples/jsm/loaders/DRACOLoader.js';

// Mock DRACOLoader to track instantiation and disposal
jest.mock('three/examples/jsm/loaders/DRACOLoader.js', () => {
return {
DRACOLoader: jest.fn().mockImplementation(() => ({
setDecoderPath: jest.fn(),
dispose: jest.fn(),
preload: jest.fn(),
})),
};
});

// Mock GLTFLoader
jest.mock('three/examples/jsm/loaders/GLTFLoader.js', () => {
return {
GLTFLoader: jest.fn().mockImplementation(() => ({
setDRACOLoader: jest.fn(),
parse: jest.fn(),
load: jest.fn(),
})),
};
});

describe('ImportManager', () => {
let importManager: ImportManager;
const clipPlanes = [
new Plane(new Vector3(0, 1, 0), 0),
new Plane(new Vector3(0, -1, 0), 0),
];
const EVENT_DATA_ID = 'EventData';
const GEOMETRIES_ID = 'Geometries';

beforeEach(() => {
// Clear all mock calls before each test
jest.clearAllMocks();
importManager = new ImportManager(clipPlanes, EVENT_DATA_ID, GEOMETRIES_ID);
});

afterEach(() => {
// Cleanup after each test
if (importManager) {
importManager.cleanup();
}
});

describe('DRACOLoader lifecycle management', () => {
it('should not create DRACOLoader until first GLTF load', () => {
// DRACOLoader should not be instantiated on construction
expect(DRACOLoader).not.toHaveBeenCalled();
expect(importManager['dracoLoader']).toBeNull();
});

it('should lazily create DRACOLoader on first getDRACOLoader call', () => {
// Access the private method to trigger lazy initialization
const dracoLoader = importManager['getDRACOLoader']();

expect(DRACOLoader).toHaveBeenCalledTimes(1);
expect(dracoLoader).toBeDefined();
expect(dracoLoader.setDecoderPath).toHaveBeenCalled();
});

it('should reuse the same DRACOLoader instance across multiple calls', () => {
// First call creates the instance
const firstLoader = importManager['getDRACOLoader']();
// Second call should return the same instance
const secondLoader = importManager['getDRACOLoader']();

expect(DRACOLoader).toHaveBeenCalledTimes(1);
expect(firstLoader).toBe(secondLoader);
});

it('should dispose DRACOLoader on cleanup', () => {
// Initialize the DRACOLoader
const dracoLoader = importManager['getDRACOLoader']();

// Call cleanup
importManager.cleanup();

// Verify dispose was called
expect(dracoLoader.dispose).toHaveBeenCalledTimes(1);
expect(importManager['dracoLoader']).toBeNull();
});

it('should handle cleanup when DRACOLoader was never initialized', () => {
// DRACOLoader should not exist yet
expect(importManager['dracoLoader']).toBeNull();

// Cleanup should not throw
expect(() => importManager.cleanup()).not.toThrow();
});

it('should allow re-initialization after cleanup', () => {
// Initialize and cleanup
importManager['getDRACOLoader']();
importManager.cleanup();

expect(DRACOLoader).toHaveBeenCalledTimes(1);
expect(importManager['dracoLoader']).toBeNull();

// Re-initialize
const newLoader = importManager['getDRACOLoader']();

expect(DRACOLoader).toHaveBeenCalledTimes(2);
expect(newLoader).toBeDefined();
});

it('should set correct decoder path for DRACOLoader', () => {
const dracoLoader = importManager['getDRACOLoader']();

// Verify setDecoderPath was called with a CDN URL
expect(dracoLoader.setDecoderPath).toHaveBeenCalledWith(
expect.stringContaining('cdn.jsdelivr.net/npm/three@'),
);
expect(dracoLoader.setDecoderPath).toHaveBeenCalledWith(
expect.stringContaining('/examples/jsm/libs/draco/'),
);
});
});

describe('GLTF loading methods use shared DRACOLoader', () => {
it('parsePhnxScene should use shared DRACOLoader', () => {
// Mock the callback
const callback = jest.fn();

// Start loading (this will fail but we're testing DRACOLoader usage)
importManager.parsePhnxScene({}, callback).catch(() => {
// Expected to fail without valid GLTF data
});

// Verify DRACOLoader was created only once
expect(DRACOLoader).toHaveBeenCalledTimes(1);
});

it('multiple GLTF operations should share the same DRACOLoader', async () => {
const callback = jest.fn();

// Simulate multiple load operations
importManager.parsePhnxScene({}, callback).catch(() => {});
importManager.parsePhnxScene({}, callback).catch(() => {});
importManager.parsePhnxScene({}, callback).catch(() => {});

// Should still only have one DRACOLoader instance
expect(DRACOLoader).toHaveBeenCalledTimes(1);
});
});

describe('Memory leak prevention', () => {
it('should not leak DRACOLoader instances on repeated operations', () => {
// Simulate the problematic pattern that was causing leaks
// Before fix: each operation created a new DRACOLoader
// After fix: all operations share one DRACOLoader

for (let i = 0; i < 10; i++) {
importManager['getDRACOLoader']();
}

// Only one DRACOLoader should have been created
expect(DRACOLoader).toHaveBeenCalledTimes(1);
});

it('should properly dispose all resources on cleanup', () => {
// Use the loader
const dracoLoader = importManager['getDRACOLoader']();

// Verify initial state
expect(importManager['dracoLoader']).not.toBeNull();

// Cleanup
importManager.cleanup();

// Verify cleanup
expect(dracoLoader.dispose).toHaveBeenCalled();
expect(importManager['dracoLoader']).toBeNull();
});
});
});

describe('ImportManager integration with ThreeManager cleanup', () => {
it('should be safe to call cleanup multiple times', () => {
const importManager = new ImportManager(
[new Plane(new Vector3(0, 1, 0), 0)],
'EventData',
'Geometries',
);

// Initialize the DRACOLoader
importManager['getDRACOLoader']();

// Multiple cleanup calls should not throw
expect(() => {
importManager.cleanup();
importManager.cleanup();
importManager.cleanup();
}).not.toThrow();
});
});