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

chore(sender): simplify error handling #19

Closed
wants to merge 1 commit into from
Closed

Conversation

glinton
Copy link

@glinton glinton commented Mar 16, 2023

The former error function in Sender would somehow swallow errors and not get them onto the channel. This method ensures that every error is delivered.

Tested with the following test:

func TestError(t *testing.T) {
	var (
		eChan   = make(chan error, 1)
		done    = make(chan struct{})
		sender  = &Sender{Errors: eChan}
		ErrNone = errors.New("thing")
		max     = 5
	)

	go func() {
		for i := 0; i < max; i++ {
			err := <-eChan
			assert.Equal(t, ErrNone, err)
		}
		close(eChan)
		done <- struct{}{}
	}()

	for i := 0; i < max; i++ {
		sender.error(ErrNone)

		// swap the following with the above call to [sender.error] for a working solution. 
		//     sender.Errors <- ErrNone
	}

	// if not running with timeout, close the channel. if working properly, this would cause a panic.
	// due to closing closed chan.
	//    close(eChan)
	<-done
}

Note:
After a discussion with @aaronfuj, I have to note that this breaks compatibility, specifically for clients that aren't reading from the error channel (causes a deadlock). If the intention of the error channel is best-effort delivery with no guarantees, go ahead and close this pr.

The former error function would somehow swallow errors and not get them
onto the channel. This ensures that every error is delivered.
@glinton glinton requested a review from aaronfuj March 16, 2023 23:06
@aaronfuj
Copy link
Contributor

aaronfuj commented Mar 17, 2023

Based on my understanding of error() and the use/purpose of this function is to submit a 'best effort' error to the channel for consumption. If there is no consumer (or a slow consumer) of the error channel, this would result in a blocking of the rest of execution until the errors had been read.

I was able to test/validate that via adjusting the error channel in your example test, i.e. eChan where if you set it to a buffered value >= max, this unit test should always complete successfully. The reason it is "failing" or dropping errors in the test is because the goroutine is not keeping up with the same rate as submission (likely due to the extra assertion code and having to read from the channel itself).

Therefore I think this is workin by design (despite the intent not being commented). Adjusting the code with the proposed change would introduce flow control or backpressure based on the errors channel, which may not be backwards compatible with existing implementations.

@glinton glinton closed this Mar 17, 2023
@glinton glinton deleted the gl-tidy01 branch March 17, 2023 00:33
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