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

Send() returns an incorrect error if the context is cancelled #309

Open
richardpark-msft opened this issue Sep 6, 2023 · 4 comments
Open

Comments

@richardpark-msft
Copy link
Member

Today you get back this error:

Error{Condition: amqp:link:transfer-limit-exceeded, Description: credit limit exceeded for sending link dx-pqVSDqo0PVdKQEeXLLDbJo25byEAW-CJCn6CxPLZEgdV5E-bDCQ, Info: map[]}

Coming from here: link

Now that we have proper rollback if sends fail/cancel I think we can safely return context.Canceled or context.DeadlineExceeded if there were no side effects worth reporting.

@jhendrixMSFT
Copy link
Member

Agreed that for a canceled context we should return context.Canceled. For context.DeadlineExceeded this might lead to ambiguities. Assuming that the sender's mux isn't abnormally blocked, failing to write to s.transfers would indicate that the peer doesn't have any credit, so I don't think we want to paper over that.

@serbrech
Copy link
Member

serbrech commented Sep 7, 2023

failing to write to s.transfers would indicate that the peer doesn't have any credit, so I don't think we want to paper over that.

if this is a legitimate case, you would still want to fail matching the deadline provided on the context.
not block indefinitely there. At which point do you return an error that knows for sure that it's because the peer doesn't have any credit? it should know that before the deadline, And return an appropriate error. but if you do reach the deadline, it should always be correct to return DeadlineExceeded.

@jhendrixMSFT
Copy link
Member

jhendrixMSFT commented Sep 7, 2023

We won't block indefinitely if there's no credit. We'll return an *amqp.Error instead of a context.DeadlineExceeded error.

You bring up a good point about knowing ahead of time if there's enough credit. I'll take a look at that.

@jhendrixMSFT
Copy link
Member

jhendrixMSFT commented Sep 7, 2023

Thinking about this more. The lack of credit should be temporal. So, I'm hesitant to just outright fail the transfer as that could be racy (i.e. the flow frame is in flight with credit, we just haven't received/processed it yet).

There is a bit of ambiguity though. There are two places where the sender waits on ctx.Done(). The one we're discussing here and waiting for message settlement. Is it important to be able to distinguish between the two cases? i.e. in the former (no credit), should apps retry sending the message? Should they retry sending if we cancel/time out waiting for message settlement?

Maybe I'm overthinking all of this and just returning ctx.Err() is good enough.

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

3 participants