Skip to content

Commit f974b40

Browse files
committed
refactor: use AsyncLock in LoadPendingIncludesAsync and remove premature PreStripConditionalIncludes calls
- Use AsyncLock (per reviewer suggestion) instead of manual _CurrentLoadingPromise logic in LoadPendingIncludesAsync; concurrent callers now serialize through the lock rather than sharing a raw promise reference - Remove PreStripConditionalIncludes calls from Process() and PreProcess(): shader .ts files are not yet regenerated with lazy loaders, so double-evaluating #ifdef blocks was causing 27 WebGL2 visualization test regressions; the function definition is preserved for when shaders are regenerated - Update unit tests to match AsyncLock async semantics and reset the static lock in beforeEach to prevent cross-test lock leaks
1 parent ed7fd97 commit f974b40

3 files changed

Lines changed: 30 additions & 42 deletions

File tree

packages/dev/core/src/Engines/Processors/shaderProcessor.ts

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,6 @@ export function Process(sourceCode: string, options: _IProcessingOptions, callba
6161
if (options.processor?.preProcessShaderCode) {
6262
sourceCode = options.processor.preProcessShaderCode(sourceCode, options.isFragment);
6363
}
64-
// Pre-strip conditional blocks so that #include directives inside disabled #ifdef blocks are never resolved.
65-
sourceCode = PreStripConditionalIncludes(sourceCode, options, engine);
6664
ProcessIncludes(sourceCode, options, (codeWithIncludes) => {
6765
if (options.processCodeAfterIncludes) {
6866
codeWithIncludes = options.processCodeAfterIncludes(options.isFragment ? "fragment" : "vertex", codeWithIncludes, options.defines);
@@ -77,8 +75,6 @@ export function PreProcess(sourceCode: string, options: _IProcessingOptions, cal
7775
if (options.processor?.preProcessShaderCode) {
7876
sourceCode = options.processor.preProcessShaderCode(sourceCode, options.isFragment);
7977
}
80-
// Pre-strip conditional blocks so that #include directives inside disabled #ifdef blocks are never resolved.
81-
sourceCode = PreStripConditionalIncludes(sourceCode, options, engine);
8278
ProcessIncludes(sourceCode, options, (codeWithIncludes) => {
8379
if (options.processCodeAfterIncludes) {
8480
codeWithIncludes = options.processCodeAfterIncludes(options.isFragment ? "fragment" : "vertex", codeWithIncludes, options.defines);

packages/dev/core/src/Engines/shaderStore.ts

Lines changed: 10 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { ShaderLanguage } from "../Materials/shaderLanguage";
2+
import { AsyncLock } from "../Misc/asyncLock";
23

34
/**
45
* Defines the shader related stores and directory
@@ -67,49 +68,31 @@ export class ShaderStore {
6768

6869
/**
6970
* @internal
70-
* Shared promise for the currently in-flight loading operation, so multiple callers
71-
* await the same work rather than duplicating loads.
71+
* Lock used to serialize concurrent calls to LoadPendingIncludesAsync so that
72+
* multiple callers don't duplicate include-loading work.
7273
*/
73-
private static _CurrentLoadingPromise: Promise<void> | null = null;
74+
private static _LoadLock = new AsyncLock();
7475

7576
/**
7677
* Call after importing shader modules to eagerly load all their include dependencies.
7778
* Each generated shader module registers a batch-loader at import time.
7879
* This method fires them all in parallel and clears the pending list.
7980
* It loops to also load nested include dependencies (include files that themselves
8081
* reference further includes push their own loaders when imported).
81-
* Concurrent callers share the same loading promise to avoid duplicate work.
82+
* Concurrent callers are serialized via AsyncLock to avoid duplicate work.
8283
* @internal
8384
*/
8485
static async LoadPendingIncludesAsync(): Promise<void> {
85-
// Fast path: nothing pending and no in-flight loading
86-
if (ShaderStore._PendingIncludesLoaders.length === 0 && !ShaderStore._CurrentLoadingPromise) {
86+
// Fast path: nothing pending
87+
if (ShaderStore._PendingIncludesLoaders.length === 0) {
8788
return;
8889
}
89-
// If loading is already in progress, wait for it then check for stragglers
90-
if (ShaderStore._CurrentLoadingPromise) {
91-
await ShaderStore._CurrentLoadingPromise;
92-
if (ShaderStore._PendingIncludesLoaders.length > 0) {
93-
await ShaderStore.LoadPendingIncludesAsync();
94-
}
95-
return;
96-
}
97-
// Start a new loading cycle that loops until all nested includes are loaded
98-
const doLoadAsync = async () => {
90+
await ShaderStore._LoadLock.lockAsync(async () => {
9991
while (ShaderStore._PendingIncludesLoaders.length > 0) {
10092
const loaders = ShaderStore._PendingIncludesLoaders.splice(0);
10193
// eslint-disable-next-line no-await-in-loop
102-
await Promise.all(loaders.map(async (fn) => await fn()));
94+
await Promise.all(loaders.map((fn) => fn()));
10395
}
104-
};
105-
const promise = doLoadAsync();
106-
ShaderStore._CurrentLoadingPromise = promise;
107-
try {
108-
await promise;
109-
} finally {
110-
if (ShaderStore._CurrentLoadingPromise === promise) {
111-
ShaderStore._CurrentLoadingPromise = null;
112-
}
113-
}
96+
});
11497
}
11598
}

packages/dev/core/test/unit/Engines/shaderStore.test.ts

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,14 @@
11
import { describe, it, expect, beforeEach, vi } from "vitest";
2+
import { AsyncLock } from "core/Misc/asyncLock";
23
import { ShaderStore } from "core/Engines/shaderStore";
34

45
describe("ShaderStore", () => {
56
describe("LoadPendingIncludesAsync", () => {
67
beforeEach(() => {
78
// Reset state between tests
89
ShaderStore._PendingIncludesLoaders.length = 0;
9-
// Force clear any in-flight promise by accessing private field
10-
(ShaderStore as any)._CurrentLoadingPromise = null;
10+
// Reset the AsyncLock so no test can leak a pending lock into the next
11+
(ShaderStore as any)._LoadLock = new AsyncLock();
1112
});
1213

1314
it("resolves immediately when no loaders are pending", async () => {
@@ -43,41 +44,49 @@ describe("ShaderStore", () => {
4344
expect(ShaderStore._PendingIncludesLoaders).toHaveLength(0);
4445
});
4546

46-
it("concurrent callers share the same loading promise", async () => {
47+
it("concurrent callers serialize via lock — loader is called exactly once", async () => {
4748
let resolveLoader: () => void;
4849
const loaderPromise = new Promise<void>((r) => (resolveLoader = r));
4950
const loader = vi.fn().mockReturnValue(loaderPromise);
5051
ShaderStore._PendingIncludesLoaders.push(loader);
5152

52-
// Start two concurrent calls
53+
// Start two concurrent callers
5354
const p1 = ShaderStore.LoadPendingIncludesAsync();
5455
const p2 = ShaderStore.LoadPendingIncludesAsync();
5556

56-
// Loader should only be called once despite two callers
57-
expect(loader).toHaveBeenCalledOnce();
58-
5957
resolveLoader!();
6058
await Promise.all([p1, p2]);
6159

60+
// Loader should be called exactly once: the second caller finds the queue empty
61+
expect(loader).toHaveBeenCalledOnce();
6262
expect(ShaderStore._PendingIncludesLoaders).toHaveLength(0);
6363
});
6464

65-
it("clears in-flight promise after completion", async () => {
65+
it("lock is released after successful completion", async () => {
6666
const loader = vi.fn().mockResolvedValue(undefined);
6767
ShaderStore._PendingIncludesLoaders.push(loader);
6868

6969
await ShaderStore.LoadPendingIncludesAsync();
7070

71-
expect((ShaderStore as any)._CurrentLoadingPromise).toBeNull();
71+
// Calling again with a new loader should work — lock must be free
72+
const loader2 = vi.fn().mockResolvedValue(undefined);
73+
ShaderStore._PendingIncludesLoaders.push(loader2);
74+
await ShaderStore.LoadPendingIncludesAsync();
75+
expect(loader2).toHaveBeenCalledOnce();
7276
});
7377

74-
it("clears in-flight promise even if a loader rejects", async () => {
78+
it("lock is released even if a loader rejects", async () => {
7579
const error = new Error("loader failed");
7680
const loader = vi.fn().mockRejectedValue(error);
7781
ShaderStore._PendingIncludesLoaders.push(loader);
7882

7983
await expect(ShaderStore.LoadPendingIncludesAsync()).rejects.toThrow("loader failed");
80-
expect((ShaderStore as any)._CurrentLoadingPromise).toBeNull();
84+
85+
// Subsequent call should not hang — lock must be free
86+
const loader2 = vi.fn().mockResolvedValue(undefined);
87+
ShaderStore._PendingIncludesLoaders.push(loader2);
88+
await ShaderStore.LoadPendingIncludesAsync();
89+
expect(loader2).toHaveBeenCalledOnce();
8190
});
8291

8392
it("second caller picks up loaders added after first batch finishes", async () => {

0 commit comments

Comments
 (0)