Skip to content

Create the first PowerShellManager instance when processing the first FunctionLoad request #201

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

Merged
merged 8 commits into from
May 6, 2019

Conversation

daxian-dbw
Copy link
Contributor

@daxian-dbw daxian-dbw commented Apr 29, 2019

This is to fix the regression in local debugging.
We create the first PowerShellManager instance when processing the first FunctionLoad request, so the debugger has the target Runspace to attach to.
However, we delay the initialization of the first PowerShellManager instance until the first invocation (when checking out the instance from pool).

It turns out we cannot have the dependency manager to create a separate Runspace for downloading.
The VS Code local debugging assumes the target Runspace has Id = 1, which was true before as only one Runspace will be created initially.
Now, the dependency manager creates a separate Runspace for downloading, and there will be a race condition -- sometimes the Runspace created for the downloading has Id = 1, and thus the debugger will attach to that Runspace, which will be reclaimed after the download and causing the Wait-Debugger in the user's function script to not be hit.

This refactoring make sure only one Runspace is created initially -- the PowerShell instance used for the downloading is also used to serve the first invocation request.
So there is no race condition and local debugging can work reliably.

Fix #196.

@daxian-dbw
Copy link
Contributor Author

@TylerLeonhardt, I made more changes on top of #200 than I thought, so I just pushed a separate PR.

TylerLeonhardt
TylerLeonhardt previously approved these changes May 1, 2019
Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM

@daxian-dbw
Copy link
Contributor Author

It turns out we cannot have the dependency manager to create a separate Runspace for downloading.
The VS Code local debugging assumes the target Runspace has Id = 1, which was true before as only one Runspace will be created initially.
Now, the dependency manager creates a separate Runspace for downloading, and there will be a race condition -- sometimes the Runspace created for the downloading has Id = 1, and thus the debugger will attach to that Runspace, which will be reclaimed after the download and causing the Wait-Debugger in the user's function script to not be hit.

This refactoring make sure only one Runspace is created initially -- the PowerShell instance used for the downloading is also used to serve the first invocation request.
So there is no race condition and local debugging can work reliably.

@daxian-dbw
Copy link
Contributor Author

@TylerLeonhardt and @SatishRanjan Can you both please review this PR again? Thanks

@daxian-dbw daxian-dbw dismissed TylerLeonhardt’s stale review May 4, 2019 17:41

Design changed, need to be reviewed again.

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM!

@Francisco-Gamino
Copy link
Contributor

Francisco-Gamino commented May 6, 2019

LGTM. Thank you @daxian-dbw for fixing the local debugging issue!

@daxian-dbw daxian-dbw merged commit 3ed9960 into Azure:dev May 6, 2019
@daxian-dbw daxian-dbw deleted the fix-no-runspace branch May 6, 2019 22:26
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.

REGRESSION: Local debugging fails - vscode and console
4 participants