Resolve follow-up comments in PR "Create default arguments during binding"#49588
Resolve follow-up comments in PR "Create default arguments during binding"#49588RikkiGibson merged 2 commits intodotnet:masterfrom
Conversation
| // For debug purposes let's instead use a token which expires after a modest amount of time | ||
| // to wait for the default syntax value to be available. | ||
| var cts = new CancellationTokenSource(TimeSpan.FromSeconds(5)); | ||
| var cts = new CancellationTokenSource(TimeSpan.FromMinutes(1)); |
There was a problem hiding this comment.
Was looking at this and thinking that even one minute probably isn't enough here. Seen CI have hiccups where threads stalled for considerable amounts of time in the past.
This lead me to think about how this problem is approached elsewhere in our code base. Specifically how is SpinWaitComplete handled when called outside a ForceComplete method chain where there is an explicit CancellationToken to use. Found the following examples:
AliasSymbol.GetTargetSourceMemberContainerSymbol.GetMembersByNameSlow
In these cases we always pass default for the CancellationToken. I think that is the approach we should take here. This means that well written code will behave correctly, even under extreme CPU load. While the current approach could exhibit incorrect behavior under extreme load.
The upside is that when this is done wrong, if we make the mistake you mention, the code will deadlock and deadlocks are very easy to debug. 😄
There was a problem hiding this comment.
Have simplified it to resemble the two examples you pointed out. I found it useful to introduce for debugging, but not worthwhile to attempt to justify the soundness of it (even if only in debug builds). Thanks!
|
I'm still unsure about how to address #49186 (comment) |
* upstream/master: (265 commits) Use extra generic type parameters and apply C#-specific knowledge to all langs instead of using inheritance Cover all changed code paths Stop removing parens that are required by C# Fix unnecessary spans Failing test for preserving parens around conditional expression AddSynthesizedRecordMembersIfNecessary - avoid touching members that are known to have no effect on the outcome of the function. (dotnet#49610) Resolve follow-up comments in PR "Create default arguments during binding" (dotnet#49588) Remove restore and checkout from test jobs (dotnet#49452) 3.8.* -> 3.9.* Update PublishData.json Update Versions.props Remove Microsoft.CodeAnalysis.VisualBasic.dll from the VSPE.OptProfTests.DDRIT_RPS_ManagedLangs_Typing runs Fix the ability to expand the list of analyzers in a reference Fix comment Address feedback to ensure `/warnaserror-:ID` prevents config options from bumping a warning to an error. parallel restore on mac/linux (dotnet#49523) VSMac: Make QuickFix preview resizable and add title (dotnet#49394) Add CallerMemberNameAttributeWithImplicitObjectCreation test (dotnet#49556) Update dependencies from https://github.com/dotnet/arcade build 20201120.10 (dotnet#49541) Clarify comment ...
Closes #49559
I wasn't sure how to resolve this comment, so will just push another commit once we've gotten that figured out. /cc @jaredpar