Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/6.0] Fix in-process decoding of DateTime in EventSource #68352

Merged
merged 3 commits into from
May 4, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Apr 21, 2022

Backport of #67557 to release/6.0

/cc @josalem

Customer Impact

@MihaZupan root caused this issue succinctly in this comment: #61563 (comment)

@tommcdon it is a bug introduced by #52092 in .NET 6.0, but only affects in-process EventListeners.

The difference is in how the serialized value (long) is de-serialized by DecodeObjects:

Sadly this was not caught by tests since none of them uses DateTime with WriteEventCore - we are only testing the varargs overload.

The fix here would be to change


to

decoded = DateTime.FromFileTimeUtc(*(long*)dataPointer);

-- MihaZupan

Testing

Test added with this PR and is running in main.

Risk

Low risk. The functional change is <1 LOC. The remainder are a test of the change.

@ghost
Copy link

ghost commented Apr 21, 2022

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #67557 to release/6.0

/cc @josalem

Customer Impact

Testing

Risk

IMPORTANT: If this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-System.Diagnostics.Tracing

Milestone: -

@josalem josalem self-assigned this Apr 21, 2022
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. We will take for consideration in 6.0.x. Please ensure to get another code review.

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@josalem
Copy link
Contributor

josalem commented Apr 27, 2022

@jeffschwMSFT should I add the servicing-consider tag or will that be done by tactics?

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Apr 27, 2022
@jeffschwMSFT jeffschwMSFT added this to the 6.0.x milestone Apr 27, 2022
@jeffschwMSFT
Copy link
Member

Added. Are the PR failures understood?

@tommcdon
Copy link
Member

Added. Are the PR failures understood?

The failures look related to TLS 1.0/1.1 disablement. I believe the test fixes have not yet been merged: #68332.

@josalem
Copy link
Contributor

josalem commented May 2, 2022

Corroborating Tom's assertion. Nearly all the test failures look like:

System.ComponentModel.Win32Exception : The client and server cannot communicate, because they do not possess a common algorithm.

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels May 3, 2022
@leecow leecow modified the milestones: 6.0.x, 6.0.6 May 3, 2022
@carlossanlop
Copy link
Member

/azp run runtime

@carlossanlop
Copy link
Member

Running the tests again now that I merged #68332 .

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@carlossanlop
Copy link
Member

Test failures unrelated, they will get disabled in this PR: #68867

@carlossanlop carlossanlop merged commit 1c6bd4d into release/6.0 May 4, 2022
@carlossanlop carlossanlop deleted the backport/pr-67557-to-release/6.0 branch May 4, 2022 19:38
@ghost ghost locked as resolved and limited conversation to collaborators Jun 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants