Fix for #2301 #2315
Fix for #2301 #2315
Conversation
|
Hi @nickwalkmsft, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! The agreement was validated by .NET Foundation and real humans are currently evaluating your PR. TTYL, DNFBOT; |
| @@ -422,7 +423,11 @@ public static void PerformBackgroundDeployment(DeploymentInfo deployInfo, IEnvir | |||
| {"method", "POST"} | |||
| }); | |||
|
|
|||
| Task.Run(() => | |||
| // For monitoring creation of temp deployment | |||
| var tcs = new TaskCompletionSource<object>(); | |||
There was a problem hiding this comment.
why not simply auto or manual reset event?
There was a problem hiding this comment.
TCS was a little "friendlier" for the situation. ARE/MRE require disposal, try/catch check for ObjectDisposedException when calling Set() (if the timeout fires first, we'll dispose it before Set() gets called), and you can await a TCS task directly, and I'd rather await than block.
| { | ||
| // Wait until the temp deployment is created before returning | ||
| var timeout = TimeSpan.FromSeconds(5); | ||
| await Task.WhenAny(tcs.Task, Task.Delay(timeout)); |
There was a problem hiding this comment.
Task.Delay will have an orphan timer in happy. It will be moot if using auto/manual reset event. If timeout what to do? Nothing if this is simply best effort to reduce (but not eliminate) the race temp deployment issue.
There was a problem hiding this comment.
After reviewing a few things, I am getting rid of the timeout. It was intended to be a safeguard against the response never being returned if something went really wrong, but now I see that the only caller that will trigger this codepath is the frontend/ARM, and its behavior heavily depends on the temp deployment being created before the response is returned. Other callers (webhooks primarily) don't set isAsync because they don't care about the deployment status, so we won't end up waiting here at all.
Instead of falling back on a timeout, it will fall back on the completion of the deployment task itself, in case it blew up early or failed to run for some odd reason.
|
|
nickwalkmsft
force-pushed
the
2301_fix
branch
from
efb5d88
to
f7c1710
Compare
Fix for #2301
For background deployments, create the temp deployment only after acquiring the deployment lock. If the deployment was requested as isAsync, wait (with timeout) for the temp deployment to be created before returning to the caller.