-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[release/2.0] Port dictionary hang detection from 2.1 #18219
Conversation
fyi @mjsabby |
Yes, looks like it. FWIW, I do not think that this is a good change to port to servicing. It has potential to break deployed apps. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The manual merge looks good to me. As Jan already expressed his doubts that this is good servicing candidate I'm just commenting here.
Servicing acts as an in place upgrade to all apps on the machine? (non-self contained) So would be a non-opt in breaking change for unconnected applications on the same machine? Merge itself looks good. |
@jkotas good point let me look into off by default. This is for a specific customer as as stop gap while they work to migrate to 2.1. |
Build failures seem unrelated
@russellhadley either of these issues look famliiar? |
Wrong Russell -- @RussKeldorph ? @dotnet-bot help please |
Welcome to the dotnet/coreclr Repository The following is a list of valid commands on this PR. To invoke a command, comment the indicated phrase on the PR The following commands are valid for all PRs and repositories. Click to expand
The following jobs are launched by default for each PR against dotnet/coreclr:release/2.0.0. Click to expand
The following optional jobs are available in PRs against dotnet/coreclr:release/2.0.0. Click to expand
Have a nice day! |
@dotnet-bot test Ubuntu x64 corefx_baseline |
This is no longer blocking as I am going to deliver a private. But if the build in relaese/2.0 is broken that seems maybe worht fixing anyway. |
Yup. Will look into porting. |
OK closing this one though. |
This is a customer request: no merge, while I seek shiproom approval. Also @maryamariyan will run corefx tests locally.
2.1 change was #16991
This was a hand merge because the implementation has changed significantly.
Note: are we missing the same protection in Remove() overloads? It contains code like
If so this is missing from master as well.