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

RetryNotify does not deal with operations that hang, even if a timeout is set #146

Open
MattCosturos opened this issue Nov 21, 2024 · 1 comment

Comments

@MattCosturos
Copy link

MattCosturos commented Nov 21, 2024

I have 0 experience developing in go, so please be gentle if I misunderstand any of the inner workings of multi-threaded go development.
However to me, it appears that RetryNotify does not deal with operations that hang, even if a timeout is set.

RetryNotify uses RetryNotifyWithTimer uses doRetryNotify

doRetryNotify
Line 87 starts a for loop.
Line 88 calls the provided operation()
This is a blocking call. It waits for operation to return, and checks the return value.
Further down is a select block to wait for ctx.Done(), or the result of a timer.
But if operation() on line 88 blocks we would never get here?

I would imagine you need some code like the following inside the for loop. Again, I have 0 experience in go, this is based off what I've learned in the past 24 hours.

	errCh := make(chan error)
	resCh := make(chan res)

	go func() {
		res, err := operation()
		if err != nil {
			errCh <- err
			return
		}
		resCh <- res
	}()

	select {
	case <-ctx.Done():
		return ctx.Err()
	case err := <- errCh
		//do the error logic here
	case res := <- resCh
		return res, nil
	}
@MattCosturos
Copy link
Author

Submitted PR #147 which keeps functionality the same (all existing test cases pass) and adds a test for a blocked operation

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

No branches or pull requests

1 participant