Skip to content

Protect execute from errors caused by 2 interleaved calls #3413

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
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Protect execute from errors caused by 2 interleaved calls #3413

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
15 changes: 14 additions & 1 deletion pkgs/dartpad_ui/lib/app/execution/view_factory/frame.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class ExecutionServiceImpl implements ExecutionService {

web.HTMLIFrameElement _frame;
late String _frameSrc;
Completer<void>? _activeExecuteCompleter;
Completer<void> _readyCompleter = Completer();

ExecutionServiceImpl(this._frame) {
Expand All @@ -37,11 +38,20 @@ class ExecutionServiceImpl implements ExecutionService {
required bool isNewDDC,
required bool isFlutter,
}) async {
if (_activeExecuteCompleter != null) {
Copy link
Member

@devoncarew devoncarew Sep 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we must have a general design issue to allow us request an execution while already performing one. The UI does disable the 'run' button when executing; perhaps this was introduced as a side effect of the genai preview feature? We may want to file an issue to track down how nested executions are possible.

In any case, guarding against it in the execution impl (here) seems reasonable. You're waiting (a bit) for the first execution to finish. I think that's reasonable; you could also just do an early exit if you see that there's a non-null completer (not start a 2nd execution). I don't have an opinion which to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case I think the double execute is from the embedding context. I tried to track down what's triggering the two events but wasn't very successful.

But given there are probably other ways to introduce the same effect, making this atomic seems like a safety measure to avoid these double calls everywhere.

I was worried about the second execute actually containing different code than the first. For example, if the embedding environment sends an "empty" request followed by a real request in quick succession. We likely just want the second one.

Maybe we could throw out the first request, that seems like a safe alternative here. I don't have any strong opinions on that solution vs the one I have here. Do you have a preference?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be simplest to do an early exit if _activeExecuteCompleter is non-null and assume that the existing completer will complete correctly in the future, but no, no real preference.

await _activeExecuteCompleter!.future.timeout(
Duration(seconds: 5),
onTimeout: () {
_activeExecuteCompleter?.complete();
},
);
}
_activeExecuteCompleter = Completer();
if (!reload) {
await _reset();
}

return _send(reload ? 'executeReload' : 'execute', {
await _send(reload ? 'executeReload' : 'execute', {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the implementation of _send, this will complete very quickly - it's not doing a lot of computation, loading resources, ... anything expensive. I'm not sure that putting a completer here will actually serialize execution requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The completer is more for the Future returned by _reset. We have to make sure _send and _reset (specifically the iframe part of _reset) happen in an atomic action.

I agree _send should be very fast so I'm not particularly worried about that one completing in time. But it can introduce an async gap so we still have to include it in the atomic block.

_reset has a timeout so that part should be fine. But if _reset throws then the first call will never complete its completer and the timeout would get triggered. So the timeout is really just a safety measure.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I missed that this also included the _reset(). You might wrap both the call to _reset, and to _send, in a try/finally, so that the completer gets finished even if reset throws an exception.

'js': _decorateJavaScript(
javaScript,
modulesBaseUrl: modulesBaseUrl,
Expand All @@ -52,6 +62,9 @@ class ExecutionServiceImpl implements ExecutionService {
if (engineVersion != null)
'canvasKitBaseUrl': _canvasKitUrl(engineVersion),
});

_activeExecuteCompleter?.complete();
_activeExecuteCompleter = null;
}

@override
Expand Down