[release/3.1] Handle Counter Polling Interval of 0

[josalem]

Backport of dotnet/runtime#53836 to release/3.1

Customer Impact

When listening to EventCounters there is a dedicated thread that is supposed to publish the metric values via EventSource on a periodic timer. However due a bug it is possible to put it into an infinite loop here. This occurs any time _pollingIntervalMilliseconds <= 0, which could occur for a few reasons:

1. The listener specified EventCounterIntervalSec=0 after previously specifying a non-zero value. We would never expect a well-behaved listener to request a 0 sec interval, but the runtime shouldn't blindly trust this input.
2. The listener enables and then disables the EventSource. Due to a race the infinite loop code could be run prior to re-checking _eventSource.IsEnabled() and it would observe _pollingIntervaMilliseconds=0

(from dotnet/runtime#53564)

Testing

The included test covers scenario 1, and manual testing was done for scenario 2.

Risk

This patch reduces overall risk by removing any looping based on user input. This should prevent scenario 1 and 2 above.

[josalem]

The test failures appear to be different from what this patch is intending to fix:

Return code:      1
Raw output file:      C:\h\w\B3E80993\w\BD5C09D1\e\tracing\eventcounter\Reports\tracing.eventcounter\gh53564\gh53564.output.txt
Raw output:
BEGIN EXECUTION
 "C:\h\w\B3E80993\p\corerun.exe" gh53564.exe 
[01:52:36.143] Setting interval to 1
[01:52:39.479] Setting interval to 0
[01:52:42.489] Setting interval to 1
[01:52:45.502] Setting ReadyToVerify
[01:52:45.507] Ready to verify
Test Failed - did not see one or more of the expected runtime counters.
Expected: 100
Actual: 1
END EXECUTION - FAILED
FAILED
Test Harness Exitcode is : 1
To run the test:
> set CORE_ROOT=C:\h\w\B3E80993\p
> C:\h\w\B3E80993\w\BD5C09D1\e\tracing\eventcounter\gh53564\gh53564.cmd

The failure I would expect from original issue would be for the test to hang after setting the interval to 1. I'm inclined to think this is a timing issue in the test when run in CI and on the older version of EventPipe without some of the performance patches in 5.

I'll update the test with something more explicit than just waiting after each step.

[janvorli]

@josalem I've just hit the failure in this test in my PR in the runtime main branch in all Linux MUSL legs. See https://dev.azure.com/dnceng/public/_build/results?buildId=1183549&view=ms.vss-test-web.build-test-results-tab.

[josalem]

Alright, let me make a quick PR to fix that test upstream and I'll update this PR as well. Looks like my times are too tight for some CI legs.

[wtgodbe]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jeffschwMSFT]

Approved. We will take for consideration in 3.1.x

[danmoseley]

fixing title to match our usual pattern...

[danmoseley]

@josalem there's a bunch of build failures on Mac

https://dev.azure.com/dnceng/9ee6d478-d288-47f7-aacc-f6e6d082ae6d/_apis/build/builds/1186771/logs/202

[danmoseley]

The test failures are the cookie ones (will be fixed by parallel PR and can be ignored) and DynamicMethodGCStress.

[josalem]

@danmoseley I saw those. They look to be related to System.Globalization.Native:

2021-06-14T21:02:55.8649200Z clang: error: linker command failed with exit code 1 (use -v to see invocation)
2021-06-14T21:02:55.8738650Z make[2]: *** [src/corefx/System.Globalization.Native/System.Globalization.Native.dylib] Error 1
2021-06-14T21:02:55.8840410Z make[1]: *** [src/corefx/System.Globalization.Native/CMakeFiles/System.Globalization.Native.dir/all] Error 2

Based on the warnings they looks like the compiler is complaining of deprecated API usage.

...
2021-06-14T21:02:55.3122780Z /usr/local/opt/icu4c/include/unicode/umachine.h:96:52: note: expanded from macro 'U_ATTRIBUTE_DEPRECATED'
2021-06-14T21:02:55.3223370Z #    define U_ATTRIBUTE_DEPRECATED __attribute__ ((deprecated))
2021-06-14T21:02:55.3325030Z                                                    ^
2021-06-14T21:02:55.3426540Z /Users/runner/work/1/s/src/corefx/System.Globalization.Native/pal_icushim.c:17:1: warning: 'ucol_setVariableTop' is deprecated [-Wdeprecated-declarations]
2021-06-14T21:02:55.3527170Z FOR_ALL_ICU_FUNCTIONS
2021-06-14T21:02:55.3628830Z ^
2021-06-14T21:02:55.3730520Z /Users/runner/work/1/s/src/corefx/System.Globalization.Native/pal_icushim.h:137:24: note: expanded from macro 'FOR_ALL_ICU_FUNCTIONS'
2021-06-14T21:02:55.3830850Z     PER_FUNCTION_BLOCK(ucol_setVariableTop, libicui18n)
2021-06-14T21:02:55.3932760Z                        ^
2021-06-14T21:02:55.4034000Z /usr/local/opt/icu4c/include/unicode/ucol.h:1275:1: note: 'ucol_setVariableTop' has been explicitly marked deprecated here
2021-06-14T21:02:55.4134750Z U_DEPRECATED uint32_t U_EXPORT2 
2021-06-14T21:02:55.4236050Z ^
2021-06-14T21:02:55.4327550Z /usr/local/opt/icu4c/include/unicode/umachine.h:116:29: note: expanded from macro 'U_DEPRECATED'
2021-06-14T21:02:55.4429510Z #define U_DEPRECATED U_CAPI U_ATTRIBUTE_DEPRECATED
...

Is it possible a compiler version or the version of the icu4c library changed? This patch shouldn't affect those at all.

I couldn't build the release/3.1 branch at all on my local mac either, with or without my change.

[danmoseley]

@josalem you're right, I see the same errors on #28179.

@dotnet/area-system-globalization can someone please advise? the build on mac has broken.

[danmoseley]

@safern @tarekgh looks similar to dotnet/runtime#39583

[danmoseley]

Yes, we need to port https://github.com/dotnet/runtime/pull/39900/files to CoreCLR release/3.1

[safern]

@danmoseley you are correct. I can port it if help is needed.

[danmoseley]

@safern would you mind? I wouldn't normally ask but this change needs to get in for an internal customer.

[tarekgh]

@danmoseley talked offline with @safern and he confirmed he is going to port the fix.

[safern]

I created: #28181

[wtgodbe]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[danmoseley]

The only failures are in System.Net.Primitives. It won't let me see the Test results (build not found?) but presumably it's the cookies issue, which will be fixed by the corefx ingestion PR that is up right now. When we merge that, we can rerun this.

Or, if we don't want to wait, maybe someone can recover the test failures and we can verify it's just the cookie ones. Then we can merge immediately.

[jeffschwMSFT]

The failures were the Mac build issue (which should be resolved) and the System.Net failures.

[wtgodbe]

There was also a failure in System.Text.RegularExpressions.Tests: https://dev.azure.com/dnceng/public/_build/results?buildId=1184134&view=ms.vss-test-web.build-test-results-tab&runId=35606250&resultId=168473&paneView=debug

I don't think we have time to get in the dependency updates from corefx/core-setup, then this. The current CI run for this is https://dev.azure.com/dnceng/public/_build/results?buildId=1189190&view=results - if that looks good I think we should merge this, and hold the dependency PRs for next month

[danmoseley]

The regex failure will be also fixed by the corefx insertion (dotnet/corefx@23e15f8)

I'm fine merging this.

[jeffschwMSFT]

I will merge and keep an eye on the run tonight.

[wtgodbe]

Note that there was a merge conflict against internal/release/3.1 in the last PR, which @safern is fixing up manually

[danmoseley]

Thank you @wtgodbe and @safern !