Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Attempt to fix download failure.#2931

Merged
sean-mcmanus merged 5 commits into
mastermicrosoft/vscode-cpptools:masterfrom
seanmcm/fixProxySupportmicrosoft/vscode-cpptools:seanmcm/fixProxySupportCopy head branch name to clipboard
Dec 17, 2018
Merged

Attempt to fix download failure.#2931
sean-mcmanus merged 5 commits into
mastermicrosoft/vscode-cpptools:masterfrom
seanmcm/fixProxySupportmicrosoft/vscode-cpptools:seanmcm/fixProxySupportCopy head branch name to clipboard

Conversation

@sean-mcmanus

@sean-mcmanus sean-mcmanus commented Dec 15, 2018

Copy link
Copy Markdown
Contributor

This explains why we have 1000 users with "Failed to download VSIX" results.

The current users of VS Code Insiders won't be able to auto-upgrade to 0.21.0-insiders until they get this fix in the non-Insiders release.

@sean-mcmanus sean-mcmanus requested a review from a team December 15, 2018 03:42
@sean-mcmanus

Copy link
Copy Markdown
Contributor Author

There's not much point in pushing this change for Insiders, because the affected users won't be able to auto-install the fix. The VS Code people are saying they think it's a bug with their stuff, but they're not able to repro it. When we release our Insiders, we should be able to see how many download success versus failures we get in telemetry.

@sean-mcmanus sean-mcmanus requested a review from a team December 17, 2018 20:47
// instead of returning early from this function scope
let config: vscode.WorkspaceConfiguration = vscode.workspace.getConfiguration();
let originalProxySupport: string = config.inspect<string>('http.proxySupport').globalValue;
while (true) { // Might need to try again with a different http.proxySupport setting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't believe this can infinite loop...I could add some extra checks in case though.

@sean-mcmanus

Copy link
Copy Markdown
Contributor Author

This change shouldn't be noticeable until a 0.21.0-insiders3 or 0.22.0-insiders.

while (true) { // Might need to try again with a different http.proxySupport setting.
try {
await util.downloadFileToDestination(buildInfo.downloadUrl, vsixPath)
.catch(() => { throw new Error('Failed to download VSIX package'); });

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why are you catching the promise and throwing a new error just to catch it in the try catch block?
You can remove the .catch(() => { throw new Error('') }); and create the error object in the catch block on line 336.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay, I was just following the pattern used by the existing code. I'll try changing it.

.catch(() => { throw new Error('Failed to download VSIX package'); });
await installVsix(vsixPath, updateChannel)
.catch((error: Error) => { throw error; });
.catch((error: Error) => { throw error; });

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also not needed.

@sean-mcmanus sean-mcmanus merged commit 4b22333 into master Dec 17, 2018
@sean-mcmanus sean-mcmanus deleted the seanmcm/fixProxySupport branch December 17, 2018 21:58
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 13, 2020
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.

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.