Skip to content

Commit

Permalink
Error when using runTask in a re-entrant way (#48235)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #48235

Recursively calling runTask is not supported, as the inner call will no-op since we're already executing the eventloop.

Currently errors are not correctly propagated, but this at least makes it so that we don't attempt to schedule the task either, which could lead to incorrect assumptions being made.

Changelog: [internal]

Reviewed By: rubennorte

Differential Revision: D67107664

fbshipit-source-id: e665a96671f4812308d87aec3b880ce2009328e2
  • Loading branch information
javache authored and facebook-github-bot committed Dec 12, 2024
1 parent 9dce262 commit c06f13e
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 1 deletion.
27 changes: 27 additions & 0 deletions packages/react-native-fantom/src/__tests__/Fantom-itest.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
*/

import 'react-native/Libraries/Core/InitializeCore';

import {createRoot, runTask} from '..';
import * as React from 'react';
import {Text, View} from 'react-native';
Expand Down Expand Up @@ -70,6 +71,32 @@ describe('Fantom', () => {

expect(completed).toBe(true);
});

// TODO: when error handling is fixed, this should verify using `toThrow`
it('should throw when running a task inside another task', () => {
let lastCallbackExecuted = 0;
runTask(() => {
lastCallbackExecuted = 1;
runTask(() => {
lastCallbackExecuted = 2;
throw new Error('Recursive runTask should be unreachable');
});
});
expect(lastCallbackExecuted).toBe(1);

runTask(() => {
queueMicrotask(() => {
lastCallbackExecuted = 3;
runTask(() => {
lastCallbackExecuted = 4;
throw new Error(
'Recursive runTask from micro-task should be unreachable',
);
});
});
});
expect(lastCallbackExecuted).toBe(3);
});
});

describe('getRenderedOutput', () => {
Expand Down
16 changes: 15 additions & 1 deletion packages/react-native-fantom/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,17 +60,31 @@ class Root {
// TODO: add an API to check if all surfaces were deallocated when tests are finished.
}

let flushingQueue = false;

/*
* Runs a task on on the event loop. To be used together with root.render.
*
* React must run inside of event loop to ensure scheduling environment is closer to production.
*/
export function runTask(task: () => void | Promise<void>) {
if (flushingQueue) {
throw new Error(
'Nested runTask calls are not allowed. If you want to schedule a task from inside another task, use scheduleTask instead.',
);
}

nativeRuntimeScheduler.unstable_scheduleCallback(
schedulerPriorityImmediate,
task,
);
global.$$JSTesterModuleName$$.flushMessageQueue();

try {
flushingQueue = true;
global.$$JSTesterModuleName$$.flushMessageQueue();
} finally {
flushingQueue = false;
}
}

// TODO: Add option to define surface props and pass it to startSurface
Expand Down

0 comments on commit c06f13e

Please sign in to comment.