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

(internal/global): Global registration mechanism does not check for nil objects #5897

Open
jmacd opened this issue Oct 18, 2024 · 0 comments
Labels
bug Something isn't working
Milestone

Comments

@jmacd
Copy link
Contributor

jmacd commented Oct 18, 2024

Description

In #5881, there was discussion about the appearance of a possible panic in the internal/global meter package. If we intend to avoid every possibility of a panic due to a misbehaving SDK, then there are other code paths we should look at fixing.

I thought I would document one of these.

Steps To Reproduce

Create an alternate metric SDK, as in the file alternate_meter_test.go from #5881. Have the SDK return nil from any one of its instrument constructors. The internal/global package has (14 cases, presently) code like:

	ctr, err := m.Float64ObservableCounter(i.name, i.opts...)
	if err != nil {
		GetErrorHandler().Handle(err)
		return
	}
	i.delegate.Store(ctr)

The problem is that ctr can be nil if the SDK misbehaves because the atomic.Value.Store() method immediately panics.

	if val == nil {
		panic("sync/atomic: store of nil value into Value")
	}

Expected behavior

If this is an unacceptable risk, then we should add tests for these 14 cases. If we agree that a misbehaving SDK is responsible for itself, and if the user installs one of those as a global, it's OK to crash, then we can put a note in the code and close this issue.

@jmacd jmacd added the bug Something isn't working label Oct 18, 2024
@MrAlias MrAlias added this to the v1.32.0 milestone Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Needs Triage
Development

No branches or pull requests

2 participants