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 variable shadowing bug #519

Merged
merged 1 commit into from
Sep 19, 2022
Merged

Fix variable shadowing bug #519

merged 1 commit into from
Sep 19, 2022

Conversation

kentquirk
Copy link
Contributor

Which problem is this PR solving?

The tip of refinery was crashing in metrics with a nil pointer error. It was puzzling why a function that was supposed to never return nil was in fact returning nil. The problem was a subtle variable shadowing issue that I only found by writing a test script and running it in the debugger:

	metric, ok = metrics[name]
	if !ok {
		// create new metric using create function and add to map
		metric := createMetric(name)
		metrics[name] = metric
	}
	lock.Unlock()
	return metric

The := on the createMetric created a new variable called metric that was scoped to the body of the if clause. The next line properly assigned it to the map and then threw it away and returned the nil pointer created on the top line.

Short description of the changes

  • Removed an unneeded :
  • Added some tests for the getOrAdd function
  • Fixed a couple of spelling errors and a linter nit

@kentquirk kentquirk requested review from a team, robbkidd, JamieDanielson and MikeGoldsmith and removed request for robbkidd September 17, 2022 00:48
@kentquirk kentquirk added type: bug Something isn't working merge at will Reviewer can merge the PR once reviewed. labels Sep 17, 2022
@kentquirk kentquirk merged commit c3ddcc2 into main Sep 19, 2022
@kentquirk kentquirk deleted the kent.shadow_bug branch September 19, 2022 13:02
ghost pushed a commit to opsramp/tracing-proxy that referenced this pull request Jul 5, 2024
## Which problem is this PR solving?

The tip of refinery was crashing in metrics with a nil pointer error. It was puzzling why a function that was supposed to never return nil was in fact returning nil. The problem was a subtle variable shadowing issue that I only found by writing a test script and running it in the debugger:

```go
	metric, ok = metrics[name]
	if !ok {
		// create new metric using create function and add to map
		metric := createMetric(name)
		metrics[name] = metric
	}
	lock.Unlock()
	return metric
```
The := on the createMetric created a new variable called `metric` that was scoped to the body of the if clause. The next line properly assigned it to the map and then threw it away and returned the nil pointer created on the top line.


## Short description of the changes

- Removed an unneeded `:`
- Added some tests for the getOrAdd function
- Fixed a couple of spelling errors and a linter nit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge at will Reviewer can merge the PR once reviewed. type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants