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

Unable to cancel client.AcceptNextSessionForQueue(..) function #17565

Closed
orzel7 opened this issue Apr 13, 2022 · 5 comments · Fixed by #17598
Closed

Unable to cancel client.AcceptNextSessionForQueue(..) function #17565

orzel7 opened this issue Apr 13, 2022 · 5 comments · Fixed by #17598
Assignees
Labels
blocking-release Blocks release bug This issue requires a change to an existing behavior in the product in order to be resolved. Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention This issue needs attention from Azure service team or SDK team Service Bus

Comments

@orzel7
Copy link

orzel7 commented Apr 13, 2022

pkg: github.com/Azure/azure-sdk-for-go/sdk/messaging/azservicebus

SDK version: v0.4.0

go version: go 1.18

What happened?

function client.AcceptNextSessionForQueue(ctx, queueName, options) is not cancellable by context, passed as first argument. For example, if I want to interrupt this function by signal or by timeout, I need to wait about 1 minute before function exits with error "context canceled".

@ghost ghost added needs-triage This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Apr 13, 2022
@ghost ghost removed the needs-triage This is a new issue that needs to be triaged to the appropriate team. label Apr 13, 2022
@jhendrixMSFT jhendrixMSFT added Client This issue points to a problem in the data-plane of the library. needs-triage This is a new issue that needs to be triaged to the appropriate team. labels Apr 13, 2022
@ghost ghost removed the needs-triage This is a new issue that needs to be triaged to the appropriate team. label Apr 13, 2022
@jhendrixMSFT jhendrixMSFT removed the question The issue doesn't require a change to the product in order to be resolved. Most issues start as that label Apr 13, 2022
@jhendrixMSFT
Copy link
Member

jhendrixMSFT commented Apr 14, 2022

I observe a hang waiting for an ack during link attachment. I'm wondering if this is due to no sessions being available (at least in my simple repro).

runtime.gopark (c:\Program Files\Go\src\runtime\proc.go:362)
runtime.selectgo (c:\Program Files\Go\src\runtime\select.go:328)
github.com/Azure/go-amqp.attachLink (c:\Users\jhendrix\go\pkg\mod\github.com\!azure\go-amqp@v0.17.4\link.go:173)
github.com/Azure/go-amqp.(*Session).NewReceiver (c:\Users\jhendrix\go\pkg\mod\github.com\!azure\go-amqp@v0.17.4\session.go:95)
github.com/Azure/azure-sdk-for-go/sdk/messaging/azservicebus.createReceiverLink (c:\git\Azure\azure-sdk-for-go\sdk\messaging\azservicebus\receiver.go:511)
github.com/Azure/azure-sdk-for-go/sdk/messaging/azservicebus.(*SessionReceiver).newLink (c:\git\Azure\azure-sdk-for-go\sdk\messaging\azservicebus\session_receiver.go:92)
github.com/Azure/azure-sdk-for-go/sdk/messaging/azservicebus.(*SessionReceiver).newLink-fm (:1)
github.com/Azure/azure-sdk-for-go/sdk/messaging/azservicebus/internal.(*AMQPLinksImpl).initWithoutLocking (c:\git\Azure\azure-sdk-for-go\sdk\messaging\azservicebus\internal\amqpLinks.go:446)
github.com/Azure/azure-sdk-for-go/sdk/messaging/azservicebus/internal.(*AMQPLinksImpl).Get (c:\git\Azure\azure-sdk-for-go\sdk\messaging\azservicebus\internal\amqpLinks.go:289)
github.com/Azure/azure-sdk-for-go/sdk/messaging/azservicebus/internal.(*AMQPLinksImpl).Retry.func2 (c:\git\Azure\azure-sdk-for-go\sdk\messaging\azservicebus\internal\amqpLinks.go:315)
github.com/Azure/azure-sdk-for-go/sdk/messaging/azservicebus/internal/utils.Retry (c:\git\Azure\azure-sdk-for-go\sdk\messaging\azservicebus\internal\utils\retrier.go:60)
github.com/Azure/azure-sdk-for-go/sdk/messaging/azservicebus/internal.(*AMQPLinksImpl).Retry (c:\git\Azure\azure-sdk-for-go\sdk\messaging\azservicebus\internal\amqpLinks.go:310)
github.com/Azure/azure-sdk-for-go/sdk/messaging/azservicebus.(*SessionReceiver).RenewSessionLock (c:\git\Azure\azure-sdk-for-go\sdk\messaging\azservicebus\session_receiver.go:220)
github.com/Azure/azure-sdk-for-go/sdk/messaging/azservicebus.newSessionReceiver (c:\git\Azure\azure-sdk-for-go\sdk\messaging\azservicebus\session_receiver.go:72)
github.com/Azure/azure-sdk-for-go/sdk/messaging/azservicebus.(*Client).AcceptNextSessionForQueue (c:\git\Azure\azure-sdk-for-go\sdk\messaging\azservicebus\client.go:265)
main.main (c:\git\jhendrixMSFT\azsbcancel\main.go:19)
runtime.main (c:\Program Files\Go\src\runtime\proc.go:250)
runtime.goexit (c:\Program Files\Go\src\runtime\asm_amd64.s:1571)

Eventually, service bus sends us a detach as the operation timed out.

@richardpark-msft is AcceptNextSessionForQueue() a blocking call until a session is available? There's no doc comment explaining the behavior.

@jhendrixMSFT
Copy link
Member

Looking at the docs for the C# implementation, I believe this is the expected behavior. We will need to update go-amqp to take a context.Context in these cases (probably should have been there from the start).

@richardpark-msft
Copy link
Member

Interesting, this would need to be up the entire chain:

// connection
conn = amqp.New()

// session
sess = conn.NewSession()

// receiver
session.NewReceiver()
// and probably sender as well)

In this particular case the "hanging" call is probably the worst offender. I don't think go-amqp itself is lazy, so adding cancellation into NewReceiver() initially and get the biggest bang for our buck there. I think this might be the first case in go-amqp where we cancel an operation that is actually in-flight with the service. So we probably want to discuss a bit.

@RickWinter RickWinter added the blocking-release Blocks release label Apr 14, 2022
@ghost ghost added the needs-team-attention This issue needs attention from Azure service team or SDK team label Apr 14, 2022
@RickWinter RickWinter added bug This issue requires a change to an existing behavior in the product in order to be resolved. and removed needs-team-attention This issue needs attention from Azure service team or SDK team labels Apr 14, 2022
@ghost ghost added the needs-team-attention This issue needs attention from Azure service team or SDK team label Apr 14, 2022
@richardpark-msft
Copy link
Member

@orzel7, glad to see you again (although sorry it's because of a bug)

I'll be taking a look at this. At a high level we'll probably have to handle this cancellation within azservicebus, rather than go-amqp (like @jhendrixMSFT and I initiallly discussed) mostly because there's some state to rollback and that's a bit easier in azservicebus at this time.

I'll update with a PR soon.

richardpark-msft added a commit that referenced this issue Apr 16, 2022
AcceptNextSessionFor(Queue|Subscription) can block for a long time (server dependent) if there are no available sessions. HOWEVER, it was intended to be cancellable, which wasn't working.

This is a simpler workaround until we get context support plumbed through go-amqp itself.

Fixes #17565
@richardpark-msft
Copy link
Member

Hi @orzel7, a fix has been merged. Our official package releases are monthly so this isn't tagged, but you can retrieve it using the commit itself:

go get github.com/Azure/azure-sdk-for-go/sdk/messaging/azservicebus@63195b99ab9a611a0e5c9cb85a3a49390f4c6855

@github-actions github-actions bot locked and limited conversation to collaborators Apr 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blocking-release Blocks release bug This issue requires a change to an existing behavior in the product in order to be resolved. Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention This issue needs attention from Azure service team or SDK team Service Bus
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants