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

clean up interactive window creation #11544

Merged
merged 2 commits into from
Oct 6, 2022
Merged

Conversation

amunger
Copy link
Contributor

@amunger amunger commented Oct 5, 2022

Part of #10808

  • Remove the excessive thread safety, the deferred creation promise should be enough to ensure duplicate IWs are not created
  • Stop subscribing to events each time a cell is added by making the restore method more idempotent

@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2022

Codecov Report

Merging #11544 (77afeda) into main (48fa7d5) will decrease coverage by 0%.
The diff coverage is 100%.

❗ Current head 77afeda differs from pull request most recent head d281721. Consider uploading reports for the commit d281721 to get more accurate results

@@          Coverage Diff           @@
##            main   #11544   +/-   ##
======================================
- Coverage     63%      63%   -1%     
======================================
  Files        477      477           
  Lines      34133    34070   -63     
  Branches    5540     5529   -11     
======================================
- Hits       21567    21501   -66     
  Misses     10505    10505           
- Partials    2061     2064    +3     
Impacted Files Coverage Δ
...teractive-window/editor-integration/codewatcher.ts 70% <ø> (-1%) ⬇️
src/interactive-window/types.ts 100% <ø> (ø)
src/interactive-window/interactiveWindow.ts 75% <100%> (+<1%) ⬆️
...rc/interactive-window/interactiveWindowProvider.ts 76% <100%> (-1%) ⬇️
src/kernels/execution/notebookUpdater.ts 88% <0%> (-8%) ⬇️
src/kernels/installer/productInstaller.node.ts 74% <0%> (-7%) ⬇️
src/kernels/helpers.node.ts 62% <0%> (-5%) ⬇️
...rc/platform/common/application/applicationShell.ts 24% <0%> (-4%) ⬇️
src/notebooks/controllers/kernelRanking/helpers.ts 58% <0%> (-2%) ⬇️
src/kernels/jupyter/jupyterKernelService.node.ts 72% <0%> (-2%) ⬇️
... and 25 more

@amunger amunger force-pushed the aamunger/IWInitialization branch from e41aeea to 77afeda Compare October 6, 2022 17:06
@amunger amunger requested a review from rebornix October 6, 2022 17:21
@amunger amunger marked this pull request as ready for review October 6, 2022 17:21
const creationInProgress = createDeferred<void>();
// Ensure we don't end up calling this method multiple times when creating an IW for the same resource.
this.pendingCreations.push(creationInProgress.promise);
Copy link
Member

Choose a reason for hiding this comment

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

Reading pendingCreation, it gave me a feeling that we can have multiple creation in parallel. However we are awaiting for all pending creations to be complete before creating new ones, does it mean that pendingCreation.length <= 1 always? If so we can probably change it to pendingCreations: Promise<void> | null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I noticed that as well, I'll change it to better express that only one creation is happening at a time

Copy link
Member

@rebornix rebornix left a comment

Choose a reason for hiding this comment

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

Looks good to me in overall. Only nick pick of how we manage pendingCreation

@amunger amunger merged commit 8f3bdb7 into main Oct 6, 2022
@amunger amunger deleted the aamunger/IWInitialization branch October 6, 2022 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants