Skip to content

Commit 32aa2ff

Browse files
adamzieladamziel
authored
[Website] fs-journal: replace recursion with a loop to stop exceeding the call stack (#2912)
1 parent ac922b6 commit 32aa2ff

File tree

2 files changed

+157
-101
lines changed

2 files changed

+157
-101
lines changed

‎packages/php-wasm/fs-journal/src/lib/fs-journal.ts‎

Lines changed: 107 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -362,118 +362,124 @@ function normalizePath(path: string) {
362362
* @returns The normalized journal.
363363
*/
364364
export function normalizeFilesystemOperations(
365-
journal: FilesystemOperation[]
365+
input: FilesystemOperation[]
366366
): FilesystemOperation[] {
367-
const substitutions: Record<number, any> = {};
368-
for (let i = journal.length - 1; i >= 0; i--) {
369-
for (let j = i - 1; j >= 0; j--) {
370-
const formerType = checkRelationship(journal[i], journal[j]);
371-
if (formerType === 'none') {
372-
continue;
373-
}
367+
let journal = input;
368+
while (true) {
369+
const substitutions: Record<number, any> = {};
370+
for (let i = journal.length - 1; i >= 0; i--) {
371+
for (let j = i - 1; j >= 0; j--) {
372+
const formerType = checkRelationship(journal[i], journal[j]);
373+
if (formerType === 'none') {
374+
continue;
375+
}
374376

375-
const latter = journal[i];
376-
const former = journal[j];
377-
if (
378-
latter.operation === 'RENAME' &&
379-
former.operation === 'RENAME'
380-
) {
381-
// Normalizing a double rename is a complex scenario so let's just give
382-
// up. There's just too many possible scenarios to handle.
383-
//
384-
// For example, the following scenario may not be possible to normalize:
385-
// RENAME /dir_a /dir_b
386-
// RENAME /dir_b/subdir /dir_c
387-
// RENAME /dir_b /dir_d
388-
//
389-
// Similarly, how should we normalize the following list?
390-
// CREATE_FILE /file
391-
// CREATE_DIR /dir_a
392-
// RENAME /file /dir_a/file
393-
// RENAME /dir_a /dir_b
394-
// RENAME /dir_b/file /dir_b/file_2
395-
//
396-
// The shortest way to recreate the same structure would be this:
397-
// CREATE_DIR /dir_b
398-
// CREATE_FILE /dir_b/file_2
399-
//
400-
// But that's not a straightforward transformation so let's just not
401-
// handle it for now.
402-
logger.warn(
403-
'[FS Journal] Normalizing a double rename is not yet supported:',
404-
{
405-
current: latter,
406-
last: former,
407-
}
408-
);
409-
continue;
410-
}
377+
const latter = journal[i];
378+
const former = journal[j];
379+
if (
380+
latter.operation === 'RENAME' &&
381+
former.operation === 'RENAME'
382+
) {
383+
// Normalizing a double rename is a complex scenario so let's just give
384+
// up. There's just too many possible scenarios to handle.
385+
//
386+
// For example, the following scenario may not be possible to normalize:
387+
// RENAME /dir_a /dir_b
388+
// RENAME /dir_b/subdir /dir_c
389+
// RENAME /dir_b /dir_d
390+
//
391+
// Similarly, how should we normalize the following list?
392+
// CREATE_FILE /file
393+
// CREATE_DIR /dir_a
394+
// RENAME /file /dir_a/file
395+
// RENAME /dir_a /dir_b
396+
// RENAME /dir_b/file /dir_b/file_2
397+
//
398+
// The shortest way to recreate the same structure would be this:
399+
// CREATE_DIR /dir_b
400+
// CREATE_FILE /dir_b/file_2
401+
//
402+
// But that's not a straightforward transformation so let's just not
403+
// handle it for now.
404+
logger.warn(
405+
'[FS Journal] Normalizing a double rename is not yet supported:',
406+
{
407+
current: latter,
408+
last: former,
409+
}
410+
);
411+
continue;
412+
}
411413

412-
if (former.operation === 'CREATE' || former.operation === 'WRITE') {
413-
if (latter.operation === 'RENAME') {
414-
if (formerType === 'same_node') {
415-
// Creating a node and then renaming it is equivalent to creating
416-
// it in the new location.
414+
if (
415+
former.operation === 'CREATE' ||
416+
former.operation === 'WRITE'
417+
) {
418+
if (latter.operation === 'RENAME') {
419+
if (formerType === 'same_node') {
420+
// Creating a node and then renaming it is equivalent to creating
421+
// it in the new location.
422+
substitutions[j] = [];
423+
substitutions[i] = [
424+
{
425+
...former,
426+
path: latter.toPath,
427+
},
428+
...(substitutions[i] || []),
429+
];
430+
} else if (formerType === 'descendant') {
431+
// Creating a node and then renaming its parent directory is
432+
// equivalent to creating it in the new location.
433+
substitutions[j] = [];
434+
substitutions[i] = [
435+
{
436+
...former,
437+
path: joinPaths(
438+
latter.toPath,
439+
former.path.substring(
440+
latter.path.length
441+
)
442+
),
443+
},
444+
...(substitutions[i] || []),
445+
];
446+
}
447+
} else if (
448+
latter.operation === 'WRITE' &&
449+
formerType === 'same_node'
450+
) {
451+
// Updating the same node twice is equivalent to updating it once
452+
// at the later time.
417453
substitutions[j] = [];
418-
substitutions[i] = [
419-
{
420-
...former,
421-
path: latter.toPath,
422-
},
423-
...(substitutions[i] || []),
424-
];
425-
} else if (formerType === 'descendant') {
426-
// Creating a node and then renaming its parent directory is
427-
// equivalent to creating it in the new location.
454+
} else if (
455+
latter.operation === 'DELETE' &&
456+
formerType === 'same_node'
457+
) {
458+
// A CREATE/WRITE followed by a DELETE on the same node.
459+
// The CREATE/WRITE is redundant.
428460
substitutions[j] = [];
429-
substitutions[i] = [
430-
{
431-
...former,
432-
path: joinPaths(
433-
latter.toPath,
434-
former.path.substring(latter.path.length)
435-
),
436-
},
437-
...(substitutions[i] || []),
438-
];
439-
}
440-
} else if (
441-
latter.operation === 'WRITE' &&
442-
formerType === 'same_node'
443-
) {
444-
// Updating the same node twice is equivalent to updating it once
445-
// at the later time.
446-
substitutions[j] = [];
447-
} else if (
448-
latter.operation === 'DELETE' &&
449-
formerType === 'same_node'
450-
) {
451-
// A CREATE/WRITE followed by a DELETE on the same node.
452-
// The CREATE/WRITE is redundant.
453-
substitutions[j] = [];
454-
455-
// The DELETE is redundant only if the node was created
456-
// in this journal.
457-
if (former.operation === 'CREATE') {
458-
substitutions[i] = [];
461+
462+
// The DELETE is redundant only if the node was created
463+
// in this journal.
464+
if (former.operation === 'CREATE') {
465+
substitutions[i] = [];
466+
}
459467
}
460468
}
461469
}
462470
}
463-
// Any substitutions? Apply them and start over.
464-
// We can't just continue as the current operation may
465-
// have been replaced.
466-
if (Object.entries(substitutions).length > 0) {
467-
const updated = journal.flatMap((op, index) => {
468-
if (!(index in substitutions)) {
469-
return [op];
470-
}
471-
return substitutions[index];
472-
});
473-
return normalizeFilesystemOperations(updated);
471+
472+
if (Object.keys(substitutions).length === 0) {
473+
return journal;
474474
}
475+
476+
journal = journal.flatMap((op, index) => {
477+
if (!(index in substitutions)) {
478+
return [op];
479+
}
480+
return substitutions[index];
481+
});
475482
}
476-
return journal;
477483
}
478484

479485
type RelatedOperationInfo = 'same_node' | 'ancestor' | 'descendant' | 'none';

‎packages/php-wasm/fs-journal/src/test/fs-journal.spec.ts‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -453,4 +453,54 @@ describe('normalizeFilesystemOperations()', () => {
453453
])
454454
).toEqual([]);
455455
});
456+
it('Normalizes long rename sequences without overflowing the stack', () => {
457+
const renameCount = 350;
458+
const journal: FilesystemOperation[] = [];
459+
for (let i = 0; i < renameCount; i++) {
460+
journal.push({
461+
operation: 'CREATE',
462+
path: `/file-${i}`,
463+
nodeType: 'file',
464+
});
465+
journal.push({
466+
operation: 'RENAME',
467+
path: `/file-${i}`,
468+
toPath: `/renamed-${i}`,
469+
nodeType: 'file',
470+
});
471+
}
472+
const normalized = normalizeFilesystemOperations(journal);
473+
expect(normalized).toEqual(
474+
Array.from({ length: renameCount }, (_, i) => ({
475+
operation: 'CREATE',
476+
path: `/renamed-${i}`,
477+
nodeType: 'file',
478+
}))
479+
);
480+
});
481+
it('Normalizes even a handful of recursive rewrites', () => {
482+
const journal: FilesystemOperation[] = [
483+
{ operation: 'CREATE', path: '/dir', nodeType: 'directory' },
484+
{ operation: 'CREATE', path: '/dir/a', nodeType: 'directory' },
485+
{
486+
operation: 'RENAME',
487+
path: '/dir',
488+
toPath: '/dir/a',
489+
nodeType: 'directory',
490+
},
491+
{ operation: 'DELETE', path: '/dir/a', nodeType: 'directory' },
492+
{
493+
operation: 'RENAME',
494+
path: '/dir/a',
495+
toPath: '/dir/a/b',
496+
nodeType: 'directory',
497+
},
498+
{ operation: 'DELETE', path: '/dir/a/b', nodeType: 'directory' },
499+
];
500+
const normalized = normalizeFilesystemOperations([...journal]);
501+
expect(normalized).toEqual([
502+
{ operation: 'CREATE', path: '/dir/a', nodeType: 'directory' },
503+
{ operation: 'CREATE', path: '/dir/a/a', nodeType: 'directory' },
504+
]);
505+
});
456506
});

0 commit comments

Comments
 (0)