Remove Logging from the LibcurlConnectionPool - cleaning routine by vhvb1989 · Pull Request #3219 · Azure/azure-sdk-for-cpp

vhvb1989 · 2022-01-06T21:23:02Z

fix: #3224

vhvb1989 · 2022-01-07T17:35:36Z

/azp run cpp - storage

azure-pipelines · 2022-01-07T17:35:53Z

Azure Pipelines successfully started running 1 pipeline(s).

vhvb1989 · 2022-01-07T19:56:02Z

/azp run cpp - storage

azure-pipelines · 2022-01-07T19:56:17Z

Azure Pipelines successfully started running 1 pipeline(s).

JeffreyRichter · 2022-01-10T20:11:11Z

Can we just have the logger no-op if it has been cleaned up?
Or, maybe we never let the logger cleanup?

ahsonkhan · 2022-01-10T20:20:34Z

Can we just have the logger no-op if it has been cleaned up?

Correct me if I am mistaken or misunderstood (which is possible), @vhvb1989, but I think it's the global/static logger instance itself that is getting cleaned-up already while Cleanup is running. So, not sure @JeffreyRichter how we can have it do no-op, since the issue doesn't seem to be at a point after the Write method was called on the logger.

antkmsft · 2022-01-10T20:31:15Z

@JeffreyRichter, I don't think there is a reliable way for the logger to know it has been cleaned up. It should already be taken care of, because mutex object does have the destructor, but we end up referencing it after it has been destroyed, so it is in an unpredictable state. Suppose we even have a destructor for the logger, and for the console logger. They could potentially have an isDestroyed static boolean. Practically, it might do the trick. But theoretically, during the statics destruction, even the boolean value can be in unpredictable state, IIRC.

antkmsft · 2022-01-11T02:03:13Z

@CaseyCarter, could something like this work? - #3231

vhvb1989 · 2022-01-11T17:11:06Z

@ahsonkhan @antkmsft please approve if you don't have more requests for this

vhvb1989 · 2022-01-11T17:39:26Z

@CaseyCarter, could something like this work? - #3231

@antkmsft , so, even if it works, would it be worth it?
The general problem would be static components that might be logging while the application is terminating/exiting.
In the case of the libcurl thread pool - cleaning routine, logging is really not that important if there's not a bug we are trying to find. And when there's a bug, the logs might not be enough :P.

ahsonkhan · 2022-01-11T20:10:30Z


+- Fixed issue [3224](https://github.com/Azure/azure-sdk-for-cpp/issues/3224) on MacOS crashing application when azure logs are turned on.


nit: remove the new line

Suggested change

- Fixed issue [3224](https://github.com/Azure/azure-sdk-for-cpp/issues/3224) on MacOS crashing application when azure logs are turned on.

- Fixed issue [3224](https://github.com/Azure/azure-sdk-for-cpp/issues/3224) on MacOS crashing application when azure logs are turned on.

ahsonkhan · 2022-01-11T20:16:07Z


 - Fixed `Azure::DateTime::Parse()` validation if the result is going to exceed `9999-12-31T23:59:59.9999999` due to time zone, leap second, or fractional digits rounding up adjustments.

+- Fixed issue [3224](https://github.com/Azure/azure-sdk-for-cpp/issues/3224) on MacOS crashing application when azure logs are turned on.


The wording of this changelog is a bit awkward to parse and read. Could we create the link to the issue like we do in Embedded C, where the issue is in the beginning?
https://github.com/Azure/azure-sdk-for-c/blob/main/CHANGELOG.md#bugs-fixed

And then we could write it something like the following:
[[3224]] Fixed intermittent crash on MacOS when logging is turned on.

ahsonkhan

Other than word-smithing the change log a little bit and removing the extra new line, looks good.

antkmsft

Branding in changelog:

MacOS => macOS.
Capitalize Azure (but better do not use it, use the wording that Ahson suggested).

antkmsft

Sorry if I kept you from submitting, I did not realize. I've tried a workaround (#3231) a few days ago, and it did not work. Your change is good, just please take the suggestions for Changelog.

It is ok to not log there, it is pretty reasonable that static objects can't do certain things during destruction (i.e. talking to other static objects that may have been already destroyed).

…ig-logs

Co-authored-by: Anton Kolesnyk <41349689+antkmsft@users.noreply.github.com>

check-enforcer · 2022-01-13T19:34:02Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaluation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run cpp - [service] - ci

ahsonkhan · 2022-01-18T19:55:58Z


+- [[#3224]](https://github.com/Azure/azure-sdk-for-cpp/issues/3224) Fixed intermittent crash on macOS when logging is turned on.


Missed removing that extra new line between the list of changes within the changelog:

Suggested change

- [[#3224]](https://github.com/Azure/azure-sdk-for-cpp/issues/3224) Fixed intermittent crash on macOS when logging is turned on.

- [[#3224]](https://github.com/Azure/azure-sdk-for-cpp/issues/3224) Fixed intermittent crash on macOS when logging is turned on.

Fixed in #3255

vhvb1989 added 3 commits January 6, 2022 19:28

cp

dc20f60

big logs testing

00dca3c

update size

4c75b7f

verbose

83de675

move libcurl mutex order

2b4da21

vhvb1989 mentioned this pull request Jan 7, 2022

Exception on [MacOS] when [AZURE_LOG_LEVEL=verbose] while performing HTTP request with [ libcurl ] #3224

Closed

vhvb1989 added 4 commits January 7, 2022 22:28

no log on cleaning

3a5633c

no log on cleaning

89bd2f2

no log on cleaning

89178f3

no log on cleaning

a8fe603

vhvb1989 marked this pull request as ready for review January 7, 2022 23:43

vhvb1989 requested review from RickWinter, ahsonkhan, antkmsft, benbp, danieljurek, gearama and weshaggard as code owners January 7, 2022 23:43

vhvb1989 self-assigned this Jan 7, 2022

vhvb1989 changed the title ~~Test big logs~~ Remove Logging from the LibcurlConnectionPool - cleaning routine Jan 7, 2022

ahsonkhan reviewed Jan 7, 2022

View reviewed changes

Comment thread sdk/core/azure-core/src/http/curl/curl.cpp

ahsonkhan reviewed Jan 7, 2022

View reviewed changes

Comment thread sdk/core/azure-core/src/http/curl/curl.cpp

ahsonkhan reviewed Jan 7, 2022

View reviewed changes

Comment thread sdk/core/azure-core/src/http/curl/curl.cpp

undo comment

a00866a

vhvb1989 requested a review from ahsonkhan January 10, 2022 17:14

antkmsft reviewed Jan 10, 2022

View reviewed changes

Comment thread sdk/core/azure-core/src/http/curl/curl.cpp

ahsonkhan added this to the [2022] February milestone Jan 10, 2022

ahsonkhan added bug This issue requires a change to an existing behavior in the product in order to be resolved. Client This issue points to a problem in the data-plane of the library. labels Jan 10, 2022

ahsonkhan reviewed Jan 10, 2022

View reviewed changes

Comment thread sdk/core/azure-core/src/http/curl/curl.cpp Outdated

cl

fc6bd11

vhvb1989 requested review from ahsonkhan and antkmsft January 11, 2022 17:10

ahsonkhan reviewed Jan 11, 2022

View reviewed changes

ahsonkhan approved these changes Jan 11, 2022

View reviewed changes

antkmsft reviewed Jan 13, 2022

View reviewed changes

antkmsft approved these changes Jan 13, 2022

View reviewed changes

vhvb1989 added 2 commits January 13, 2022 19:15

Merge branch 'main' of github.com:Azure/azure-sdk-for-cpp into test-b…

9805f36

…ig-logs

cl

f712c36

vhvb1989 enabled auto-merge (squash) January 13, 2022 19:18

antkmsft requested changes Jan 13, 2022

View reviewed changes

Comment thread sdk/core/azure-core/CHANGELOG.md Outdated

Comment thread sdk/core/azure-core/src/http/curl/curl.cpp Outdated

vhvb1989 and others added 2 commits January 13, 2022 19:22

missing wording

3758bce

Update sdk/core/azure-core/src/http/curl/curl.cpp

6c23d93

Co-authored-by: Anton Kolesnyk <41349689+antkmsft@users.noreply.github.com>

antkmsft approved these changes Jan 13, 2022

View reviewed changes

vhvb1989 merged commit 30c5d77 into Azure:main Jan 13, 2022

ahsonkhan reviewed Jan 18, 2022

View reviewed changes

ahsonkhan mentioned this pull request Jan 18, 2022

Fix up the Azure Core changelog for breaking change and new line #3255

Merged


		- Fixed issue [3224](https://github.com/Azure/azure-sdk-for-cpp/issues/3224) on MacOS crashing application when azure logs are turned on.


		- Fixed `Azure::DateTime::Parse()` validation if the result is going to exceed `9999-12-31T23:59:59.9999999` due to time zone, leap second, or fractional digits rounding up adjustments.

		- Fixed issue [3224](https://github.com/Azure/azure-sdk-for-cpp/issues/3224) on MacOS crashing application when azure logs are turned on.


		- [[#3224]](https://github.com/Azure/azure-sdk-for-cpp/issues/3224) Fixed intermittent crash on macOS when logging is turned on.

Conversation

vhvb1989 commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vhvb1989 commented Jan 7, 2022

Uh oh!

azure-pipelines Bot commented Jan 7, 2022

Uh oh!

vhvb1989 commented Jan 7, 2022

Uh oh!

azure-pipelines Bot commented Jan 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JeffreyRichter commented Jan 10, 2022

Uh oh!

Uh oh!

ahsonkhan commented Jan 10, 2022

Uh oh!

antkmsft commented Jan 10, 2022

Uh oh!

antkmsft commented Jan 11, 2022

Uh oh!

vhvb1989 commented Jan 11, 2022

Uh oh!

vhvb1989 commented Jan 11, 2022

Uh oh!

ahsonkhan Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

antkmsft left a comment

Choose a reason for hiding this comment

Uh oh!

antkmsft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

check-enforcer Bot commented Jan 13, 2022

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

Uh oh!

ahsonkhan Jan 18, 2022

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Jan 18, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vhvb1989 commented Jan 6, 2022 •

edited

Loading

ahsonkhan Jan 11, 2022 •

edited

Loading