Skip to content

Conversation

@moonyoungCHAE
Copy link
Contributor

In silence bulk import, I noticed that if a decoding error occurs, the number of goroutines remains equal to the number of workers which is worker count. This happened because the channel wasn’t closed when an error occurred and the function exited. I fixed this issue by ensuring the channel is closed even in error cases.

I tested like this.

func TestName(t *testing.T) {
	var err error
	alertmanagerURL, err = url.Parse("http://example.com")
	if err != nil {
		log.Fatal(err)
	}

	goroutineCount := runtime.NumGoroutine()
	println("before bulk import ", goroutineCount) // print 2
	cmd := silenceImportCmd{
		force:   false,
		workers: 10,
		file:    "./test.json",
	}
	cmd.bulkImport(context.TODO(), nil)

	for i := 0; i < 100; i++ {
		goroutineCount := runtime.NumGoroutine()
		println(goroutineCount) // as-is: 13 (worker count), to-be: 2
		time.Sleep(1 * time.Second)
	}
}
[
  {
    "ID": "123",
    "Comment": "First silence",
    "CreatedBy": "user1",
    "EndsAt": 1 // this makes decode error
  },
  {
    "ID": "456",
    "Comment": "Second silence",
    "CreatedBy": "user2"
  }
]

Signed-off-by: moonyoung <[email protected]>
Signed-off-by: moonyoung <[email protected]>
Signed-off-by: moonyoung <[email protected]>
Copy link
Contributor

@Spaceman1701 Spaceman1701 left a comment

Choose a reason for hiding this comment

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

LGTM, this seems reasonable. Since bulkImport returning causes the program to exit, I don't think this has much impact. However, it is a little smelly to leave these workers around and I guess some future code change could call bulkImport as part of a workflow.

@Spaceman1701
Copy link
Contributor

I am 99.9% sure the test failing is unrelated. I think a real maintainer can rerun that before merging.

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.

2 participants