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

add missing waiter retry breakout on non-nil non-matched error #561

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

lucix-aws
Copy link
Contributor

Needed for aws/aws-sdk-go-v2#2937

From waiter workflow step 4:

If none of the acceptors are matched and an error was encountered while calling the operation, then transition to the failure state and stop waiting.

Our implementation, for reasons probably lost to time, omits this step, causing waiters to spin longer than they should. This change adds that missing behavior.

@lucix-aws lucix-aws requested review from a team as code owners January 9, 2025 18:23
@Madrigal
Copy link
Contributor

Madrigal commented Jan 9, 2025

From aws/aws-sdk-go-v2#2937

The code is currently


func objectExistsStateRetryable(ctx context.Context, input *HeadObjectInput, output *HeadObjectOutput, err error) (bool, error) {

	if err == nil {
		return false, nil
	}

	if err != nil {
		var errorType *types.NotFound
		if errors.As(err, &errorType) {
			return true, nil
		}
	}

	return true, nil
}

If we add if err != nil { return false, err } right before the last statement, wouldn't it contradict the first statement on the waiter?

func objectExistsStateRetryable(ctx context.Context, input *HeadObjectInput, output *HeadObjectOutput, err error) (bool, error) {

	if err == nil {
		return false, nil
	}

	if err != nil {
		var errorType *types.NotFound
		if errors.As(err, &errorType) {
			return true, nil
		}
	}
        if err != nil {
                return false, err
        }
        // this is equivalent to `if err == nil` and it was already covered 
        // at the beginning, where we returned "false, nil"
	return true, nil
}

@Madrigal
Copy link
Contributor

Madrigal commented Jan 9, 2025

Discussed offline. The first if err == nil { return false, nil} is due to the acceptor on the S3 model, where we specify that if no error is found, we don't need to retry. The 2nd equivalent statement if err == nil {return true, nil} is due to waiters retrying if there is no error found.

It still bothers me a bit to see two ifs with the same condition but different logic, but it's just an artifact of codegen and nothing to worry about

@lucix-aws lucix-aws merged commit 5e16ee7 into main Jan 9, 2025
11 checks passed
@lucix-aws lucix-aws deleted the fix-waitererror branch January 9, 2025 20:49
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.

3 participants