[testing] Update TraceEvent package version and bypass corner case in EventPipe #1794
Conversation
|
Hmm it seems like it failed with this fix as well: |
|
@josalem this is still happening and a lot of PRs are hitting it more frequently, are you looking into why? |
|
Yes, I'm looking into it. Any idea when the frequency spiked? I'm trying to see if anything might have changed that is causing this to resurface. It may be a different issue since it isn't hitting the infrastructure in place to light up when this particular bug was happening. |
|
The frequency spiked after I merged my PR to use a single pipeline yesterday. The only difference I can see is that now those tests are using a |
|
I think I've diagnosed the issue. The issue being hit now is new and not the same as the issue this PR will fix. The new issue is happening because of changes in Regex Caching that now use ConcurrentDictionaries (#542). |
|
So this seems like a regression on: #542, right? or should cc: @stephentoub who did those changes. |
|
Can you point me to where it's being used? |
|
I don't believe this is an issue with #542, but rather an issue in EventPipe that's appears as an unfortunate byproduct of those changes. A fix would happen in the EventPipe code. |
|
Maybe. Looking at it again, I think the dictionary in Regex should be lazily created when it's first needed; that way anyone using the instance rather than static Regex methods won't need to create it. And if I change that, and EventPipe is using the instance methods, this shouldn't happen. (Of course it'd be even better if EventPipe were reliable in the face of such a usage.) |
|
@stephentoub I don't think it's the fault of your changes, but here are the deadlocked callstacks: Reader (managed): Read (Native): Writer: |
|
After chatting with @sywhang there may be a quick band-aid change we can make that should clear up CI. We would need to change to use the instance methods. That should bypass the issue for now, and let us get CI back to green. An alternative that I'm going to try with this PR is to just put a Neither fixes the underlying issue, but would hopefully get CI green again while I engineer a broader fix. We could also disable the tests while I engineer the fix. |
* Remove this change once the fix has be made
This is what I was alluding to in #1794 (comment). However, right now I'm not sure that would actually help. We currently construct the ConcurrentDictionary as part of initializing a readonly static field, so it's going to be created at some point before first use, and it's possible using the instance methods could trigger it. To address that, we would need to lazily initialize the CD the first time code actually wants to add to the cache, which we could do, and maybe should. |
josalem
force-pushed
the
dev/josalem/update-traceevent-ver
branch
from
73b0e5b
to
47daa07
Compare
|
Creating the |
josalem
changed the title
|
Merging to unblock other PRs as this is green and approved. |
ghost
locked as resolved and limited conversation to collaborators
The fix (microsoft/perfview#1047) that resolves dotnet/coreclr#26241 is in version 2.0.48 of TraceEvent, so the failure can still occur randomly. This change updates the version used in testing to include the fix.
resolves dotnet/coreclr#26241
CC @tommcdon