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

fix: enricher.Instance() returns nil instead of e if e is not initialized #53

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

nddq
Copy link
Contributor

@nddq nddq commented Mar 15, 2024

Even if the enricher is not intialized, assigning enricher.Instance() = nil to the plugin's enricher interface will not result in p.enricher == nil equals to true. This is because in Go, an interface holding a nil value is not nil ref . We should not be checking for nil interface with this method.

@nddq nddq added the type/fix Fixes something label Mar 15, 2024
@nddq nddq self-assigned this Mar 15, 2024
@nddq nddq requested a review from a team as a code owner March 15, 2024 17:41
pkg/enricher/enricher.go Show resolved Hide resolved
@timraymond
Copy link
Member

There's something that makes me uncomfortable about how this is being used as a singleton, yet we are passing contexts that get discarded into New. I feel like that has a potential to leak goroutines if the first caller's context is disposed of.

Out of curiosity, why does the Enricher need to be a singleton?

@nddq
Copy link
Contributor Author

nddq commented Mar 15, 2024

There's something that makes me uncomfortable about how this is being used as a singleton, yet we are passing contexts that get discarded into New. I feel like that has a potential to leak goroutines if the first caller's context is disposed of.

Out of curiosity, why does the Enricher need to be a singleton?

this predates my time at this team 🙂 this is just a band aid solution, maybe @anubhabMajumdar can chime in

@nddq nddq merged commit 0f03236 into main Mar 15, 2024
19 checks passed
@nddq nddq deleted the nddq/enricher branch March 15, 2024 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/fix Fixes something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants