Skip to content
This repository has been archived by the owner on Oct 12, 2023. It is now read-only.

Use amqp HandleMessage Func #207

Merged
merged 3 commits into from
Feb 24, 2021

Conversation

serbrech
Copy link
Member

@serbrech serbrech commented Feb 10, 2021

Make use of the new HandleMessage api in go-amqp

This bumps the version of go-amqp to include the following bug fixes:

fixes #189

@serbrech serbrech changed the title Use amqp HandleMessage Func [WIP] Use amqp HandleMessage Func Feb 10, 2021
go.mod Outdated Show resolved Hide resolved
@@ -216,24 +214,14 @@ func (r *Receiver) Listen(ctx context.Context, handler Handler) *ListenerHandle
ctx, span := r.startConsumerSpanFromContext(ctx, "sb.Receiver.Listen")
defer span.End()

messages := make(chan *amqp.Message)
go r.listenForMessages(ctx, messages)
go r.handleMessages(ctx, messages, handler)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the handleMessages is now folded into the amqpAdapterHandler that wraps the user handler

@@ -216,24 +214,14 @@ func (r *Receiver) Listen(ctx context.Context, handler Handler) *ListenerHandle
ctx, span := r.startConsumerSpanFromContext(ctx, "sb.Receiver.Listen")
defer span.End()

messages := make(chan *amqp.Message)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no more channel to manage. we don't need an implicit buffer. prefetch messages are already buffered in go-amqp


return &ListenerHandle{
r: r,
ctx: ctx,
}
}

func (r *Receiver) handleMessages(ctx context.Context, messages chan *amqp.Message, handler Handler) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need since we pass it down to the handler without the buffer channel

}
receiver = r.receiver
r.clientMu.RUnlock()
msg, err := receiver.Receive(ctx)
err := receiver.HandleMessage(ctx, func(message *amqp.Message) error {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call HandleMessage instead of Receive which we deprecate

go.mod Show resolved Hide resolved
@serbrech serbrech changed the title [WIP] Use amqp HandleMessage Func Use amqp HandleMessage Func Feb 12, 2021
go.mod Show resolved Hide resolved
@serbrech
Copy link
Member Author

I would prefer this PR to merge in go-amqp : Azure/go-amqp#23
I can then update this PR to point at the latest go-amqp (new patch release after above PR merges)

@jhendrixMSFT jhendrixMSFT merged commit f536d5c into Azure:master Feb 24, 2021
@Segflow
Copy link

Segflow commented Mar 29, 2021

Using this version instead of 0.10.8 broke our services. Seems like using amqp HandleMessage do something different that what was used before this.

More info here: google/go-cloud#2980

@jhendrixMSFT
Copy link
Member

@Segflow are you calling a disposition action after processing the message? This is a requirement per the AMQP protocol. Earlier versions didn't honor this and caused the problems described in Azure/go-amqp#20

@Segflow
Copy link

Segflow commented Mar 29, 2021

gocloud.dev fetch the messages and store them in a slice, and return from the handler.

Later the user decides if they want to Ack or Nack the message.

@serbrech
Copy link
Member Author

this was the implementation before, and it did not allow to fulfil flow control in AMQP, which needs to communicate back to the upstream peer.
this is now respecting flow control.
The problem is that your application does not set prefetch, so it's at 1.
as long as no message disposition is taken, the server won't send you the next one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Long messages and "Servicebus Lock Duration" (go-amqp)
3 participants