Fix an issue parsing promises #1927
Conversation
We changed the function `getPromise` to not use a lock in version 2.1.2. This was a mistake. I thought it would return the unresolved `Promise` object if it was a sync function. That is not the behavior, instead it will skip that line of code and return null even if the promise exists. This causes multiple processes to try to grab the installer file handle. When one fails to grab the file handle, it causes a chain-reaction of cascading failures for concurrent requests, because that is supposed to be blocked from happening. In addition, the check I added to kill the sudo process once it is finished runs on all platforms despite the master sudo process only being present on Linux, which has resulted in some higher timeout rates. We need to not do this on other platforms.
vscode-dotnet-runtime-library/src/Acquisition/InstallTrackerSingleton.ts
Show resolved
Hide resolved
| @@ -225,7 +225,7 @@ export class WebRequestWorker | |||
| } | |||
| catch(error : any) | |||
| { | |||
| if(error?.message?.contains('ENOSPC')) | |||
| if(error && error?.message && (error?.message as string)?.includes('ENOSPC')) | |||
There was a problem hiding this comment.
Does the null coalescing operator work the same as C#? Meaning error && error?.message is the same as just error?.message. If it is, you should only need the end part, (error?.message as string)?.includes('ENOSPC') because if any part returns null, the whole thing is null, which is false.
There was a problem hiding this comment.
It does, that new check is unnecessary, but I don't want to change it for this release since we have a build. I will change it after this going forward :)
I thought maybe it was not behaving correctly because it was calling .contains on the object but the problem was .contains does not exist on a string type, not that it was trying .contains on a null type.. so this is an artifact of me writing draft code, which is not the best to keep in prod.
I dont know why I thought it was .contains instead of .includes
Bumps version to 2.1.3.
We changed the function
getPromiseto not use a lock in version 2.1.2. This was a mistake. I thought it would return the unresolvedPromiseobject if it was a sync function. That is not the behavior, instead it will skip that line of code and return null even if the promise exists. This causes multiple processes to try to grab the installer file handle.When one fails to grab the file handle, it causes a chain-reaction of cascading failures for concurrent requests, because that is supposed to be blocked from happening.
In addition, the check I added to kill the sudo process once it is finished runs on all platforms despite the master sudo process only being present on Linux, which has resulted in some higher timeout rates. We need to not do this on other platforms.