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.

Sign up for GitHub

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

Jump to bottom

Fix an issue when the error is nil but is different than nil #11335

Closed
wants to merge 1 commit into from
Closed

Fix an issue when the error is nil but is different than nil #11335

wants to merge 1 commit into from

Conversation

bogdandrutu
Copy link
Member

No description provided.

@bogdandrutu bogdandrutu requested a review from a team as a code owner October 3, 2024 00:43
@codecov Codecov
Copy link

codecov bot commented Oct 3, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.54%. Comparing base (69ff46b) to head (962cb00).
Report is 45 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #11335      +/-   ##
==========================================
+ Coverage   91.50%   91.54%   +0.03%     
==========================================
  Files         430      428       -2     
  Lines       20205    20235      +30     
==========================================
+ Hits        18489    18524      +35     
+ Misses       1341     1337       -4     
+ Partials      375      374       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

songy23

This comment was marked as resolved.

Sign in to view

@songy23 songy23 added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Oct 3, 2024
@bogdandrutu bogdandrutu force-pushed the fix-error-nil-issue branch 2 times, most recently from 0829833 to f2fde25 Compare October 3, 2024 15:02
@bogdandrutu
Fix an issue when the error is nil but is different than nil
962cb00 
Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
codeboten
codeboten reviewed Oct 3, 2024
View reviewed changes
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Is this needed everywhere .Error() is called?

@bogdandrutu bogdandrutu mentioned this pull request Oct 4, 2024
Merged
@bogdandrutu
Copy link
Member Author

bogdandrutu commented Oct 4, 2024 •
edited
Loading

Is this needed everywhere .Error() is called?

I don't think this is scalable. I think we should probably do:

  1. Add a check in mdatagen for concurrency to all components.
  2. Productionize something like Add debug wrapper for all metrics components, fix error nil #11346 and test at runtime that each component does not return bogus error.

@bogdandrutu bogdandrutu marked this pull request as draft October 4, 2024 06:13
bogdandrutu added a commit that referenced this pull request Oct 4, 2024
@bogdandrutu
Fix data race condition, concurrent writes to the err variable, cause…
d6cd593 
…s Undefined Behavior (#11349)

The main issue is that after
#10910 the
err variable is shared between requests because it uses the same address
as the err defined outside the func.

This is an UB because we are overwriting memory and will cause crashes
like
#11335,
where the check for not nil happens then gets overwrite with nil and
crashes.

Fixes
#11350

---------

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
jmacd
jmacd reviewed Oct 4, 2024
View reviewed changes
Copy link
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

Looks good. It would be nice to know which piece of code is responsible for the nil-error object and try to fix it.

@bogdandrutu
Copy link
Member Author

Looks good. It would be nice to know which piece of code is responsible for the nil-error object and try to fix it.

It was the race condition we fixed last night. #11349

@narcis96 narcis96 mentioned this pull request Oct 7, 2024
Closed
jackgopack4 pushed a commit to jackgopack4/opentelemetry-collector that referenced this pull request Oct 8, 2024
@bogdandrutu @jackgopack4
Fix data race condition, concurrent writes to the err variable, cause…
de80d20 
…s Undefined Behavior (open-telemetry#11349)

The main issue is that after
open-telemetry#10910 the
err variable is shared between requests because it uses the same address
as the err defined outside the func.

This is an UB because we are overwriting memory and will cause crashes
like
open-telemetry#11335,
where the check for not nil happens then gets overwrite with nil and
crashes.

Fixes
open-telemetry#11350

---------

Signed-off-by: Bogdan Drutu <bogdandrutu@gmail.com>
@codeboten
Copy link
Contributor

Is this PR still needed now that the race condition has been fixed?

@bogdandrutu
Copy link
Member Author

No. I don't think the PR is needed, maybe if we can get #11335 (comment) into an issue and decide what guard we want in the future.

@codeboten codeboten mentioned this pull request Oct 10, 2024
Open
@codeboten
Copy link
Contributor

Opened #11407 to track adding concurrency tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmacd jmacd jmacd left review comments

@codeboten codeboten codeboten left review comments

@andrzej-stencel andrzej-stencel andrzej-stencel approved these changes

@songy23 songy23 songy23 approved these changes

@mx-psi mx-psi mx-psi approved these changes

Assignees
No one assigned
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bogdandrutu @codeboten @jmacd @mx-psi @songy23 @andrzej-stencel