cloudbeat icon indicating copy to clipboard operation
cloudbeat copied to clipboard

Listen to ctx.Done() when sending to channels

Open amirbenun opened this issue 1 year ago • 1 comments

Background

A bug that we found in CNVM but can be relevant all across:

  • https://github.com/elastic/cloudbeat/issues/1097

Describe the bug

The pattern of sending an event on a channel without knowing who or when it is going to be consumed. Examples outside of the vulnerability flavor in pipeline.go:

	go func() {
		defer close(outputCh)
		defer cancel()

		for s := range inputChannel {
			val, err := fn(ctx, s)
			if err != nil {
				log.Error(err)
				continue
			}
			outputCh <- val
		}
	}()

Since outputCh is consumed outside of this section, sending to outputCh is not safe. If for example the consumer will stop listening to this channel (because the context canceled) it will result in deadlock. An alternative is to replace outputCh <- val with

select {
    case <- ctx.Done():
        return
    case outputCh <- val:
}

Definition of done

  • [ ] Map the sections in our source code who are in risk
  • [ ] Suggest a fix where possible
  • [ ] Add unit tests that fail in the current state and pass after the fix

amirbenun avatar Jul 03 '23 10:07 amirbenun

Thanks @amirbenun for the detailed explanation. This bug only emphasizes the need for a shared "pipeline" utility across the project.

uri-weisman avatar Jul 03 '23 11:07 uri-weisman