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 memory leak #453

Merged
merged 1 commit into from
Aug 3, 2022
Merged

Conversation

attilakunelwood
Copy link
Contributor

@attilakunelwood attilakunelwood commented Aug 2, 2022

This commit fixes a memory leak that happens if the PagerDuty endpoint returns a 429 HTTP status code. It was not verified whether the error happens with other status codes as well.

I verified the correctness of this fix by running the following test:

func TestMemoryLeak(t *testing.T) {
	evt := &pagerduty.V2Event{
		RoutingKey: routingKey,
		Action:     "trigger",
		Payload: &pagerduty.V2Payload{
			Summary:   "foo",
			Source:    "bar",
			Component: serviceName,
			Severity:  "critical",
			Class:     "",
			Details:   nil,
		},
	}

	for i := 0; i < 100; i++ {
		client.ManageEvent(evt)
		fmt.Printf("num goroutines: %d\n", runtime.NumGoroutine())
		time.Sleep(time.Millisecond * 50)
	}
}

Without the fix, this produces the following output:

num goroutines: 6
num goroutines: 8
num goroutines: 10
num goroutines: 12
num goroutines: 14
num goroutines: 16
num goroutines: 18
num goroutines: 20
num goroutines: 22
num goroutines: 24
...

With the fix, this produces the following output:

num goroutines: 8
num goroutines: 8
num goroutines: 8
num goroutines: 8
num goroutines: 8
num goroutines: 8
num goroutines: 6
num goroutines: 6
num goroutines: 6
num goroutines: 6
num goroutines: 5
num goroutines: 6
...

This commit fixes a memory leak that happens if the PagerDuty endpoint returns a 429 HTTP status code. It was not verified whether the error happens with other status codes as well.
@attilakunelwood attilakunelwood changed the title fixed memory leak Fix memory leak Aug 2, 2022
Copy link
Contributor

@acloudgopher-pd acloudgopher-pd left a comment

Choose a reason for hiding this comment

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

Thank you for the fix!

@acloudgopher-pd acloudgopher-pd merged commit eae029d into PagerDuty:master Aug 3, 2022
@ChuckCrawford
Copy link
Collaborator

Yeah thank you for the fix @attilakunelwood! We are going to have one more look at this and see if there isn't a more centralized location for us to put this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants