Skip to content
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.

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

# Fix Suggest dropdown/inline completion partial accept (via next token/line) race #197633

Merged
merged 4 commits into from
Nov 7, 2023

Conversation

marrej
Copy link
Contributor

@marrej marrej commented Nov 7, 2023

Repro steps:

monaco playground

Note that this is a race and does not trigger every time in the playground, but may be amplified by fullter event loop (running in vscode) offsetting the partialAccept trigger execution after the SuggestDropdown clear.

(Add breakpoints on the free & acceptPartial completion)

  1. type e in the already existing consol
  2. Once suggest dropdown console appears, accept next token/next line (e.g. ctrl+right)
  3. Observe the order of events. The free completion for the id of the suggestion is called before the partialAccept

expected:
The partialAccept is called before free, otherwise the extHost reference to the item will get disposed prematurely.

The following snippet fixes it quickly by increasing the ref count to active completions (containing also standard inlineCompletions for consistency), and disposing after the partialAccept finished.

Note: If the onHide functionality is being implemented, the same may be required for the Accept as the reference to the active InlineCompletion might be dropped at the time the Hide event is triggered.

closes #195385

@hediet
Copy link
Member

hediet commented Nov 7, 2023

Thanks for this very deep investigation and the explanations!

What I don't understand at a first glance though, is why there is a race condition in the first place.
This code runs in sync, so I don't understand how timing can influence the behavior.

Actually, I don't think it is a race condition. Here is the stack from the repro:

freeInlineCompletions (VM264:158) - Deletes the inline completion
removeRef (provideInlineCompletions.ts:180)
dispose (provideInlineCompletions.ts:157)
dispose (inlineCompletionsSource.ts:231)
_setValue (base.ts:453)
set (base.ts:416)
clearSuggestWidgetInlineCompletions (inlineCompletionsSource.ts:116)
(anonymous) (inlineCompletionsModel.ts:123)
transaction (base.ts:228)
(anonymous) (inlineCompletionsModel.ts:118)
_recomputeIfNeeded (derived.ts:195)
get (derived.ts:172)
reportChanges (base.ts:153)
endUpdate (utils.ts:270)
endUpdate (derived.ts:252)
finish (base.ts:266)
transaction (base.ts:230)
update (suggestWidgetInlineCompletionProvider.ts:143)
(anonymous) (suggestWidgetInlineCompletionProvider.ts:105)
_deliver (event.ts:1121)
_deliverQueue (event.ts:1132)
fire (event.ts:1156)
hideWidget (suggestWidget.ts:748) - Suggest Widget hides
(anonymous) (suggestController.ts:272)
_deliver (event.ts:1121)
_deliverQueue (event.ts:1132)
fire (event.ts:1156)
cancel (suggestModel.ts:317)
_onNewContext (suggestModel.ts:738)
_refilterCompletionItems (suggestModel.ts:441)
(anonymous) (suggestModel.ts:202)
_deliver (event.ts:1121)
_deliverQueue (event.ts:1132)
fire (event.ts:1156)
(anonymous) (codeEditorWidget.ts:1656)
_deliver (event.ts:1121)
fire (event.ts:1152)
_emitOutgoingEvents (viewModelEventDispatcher.ts:64)
endEmitViewEvents (viewModelEventDispatcher.ts:109)
_withViewEventsCollector (viewModelImpl.ts:1092)
_executeCursorEdit (viewModelImpl.ts:1029)
executeEdits (viewModelImpl.ts:1032)
executeEdits (codeEditorWidget.ts:1215) - Edits are applied
(anonymous) (inlineCompletionsModel.ts:409)
(anonymous) (nls.ts:4)
__awaiter (nls.ts:4)
_acceptNext (inlineCompletionsModel.ts:376)
(anonymous) (inlineCompletionsModel.ts:338)
(anonymous) (nls.ts:4)
__awaiter (nls.ts:4)
acceptNextWord (inlineCompletionsModel.ts:337) - acceptNextWord
(anonymous) (commands.ts:102)
(anonymous) (nls.ts:4)
__awaiter (nls.ts:4)
run (commands.ts:100)
runEditorCommand (editorExtensions.ts:386)
(anonymous) (editorExtensions.ts:320)
(anonymous) (editorExtensions.ts:315)
invokeFunction (instantiationService.ts:68)
invokeWithinContext (codeEditorWidget.ts:434)
runEditorCommand (editorExtensions.ts:308)
runCommand (editorExtensions.ts:320)
handler (editorExtensions.ts:154)
invokeFunction (instantiationService.ts:68)
executeCommand (standaloneServices.ts:285)
_doDispatch (abstractKeybindingService.ts:327)
_dispatch (abstractKeybindingService.ts:184)
(anonymous) (standaloneServices.ts:327)

@hediet hediet enabled auto-merge November 7, 2023 13:49
@hediet hediet added this to the November 2023 milestone Nov 7, 2023
@hediet hediet merged commit e456d35 into microsoft:main Nov 7, 2023
6 checks passed
amunger pushed a commit that referenced this pull request Nov 9, 2023
# Fix Suggest dropdown/inline completion partial accept (via next token/line) race
@github-actions github-actions bot locked and limited conversation to collaborators Dec 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InlineCompletion partialAccept not firing correctly with Suggest dropdown
3 participants