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.

Sign up for GitHub

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

Jump to bottom

pubsub: With exactly once delivery and message ordering enabled, message order is not mantained #9440

Closed
MiquelPiza opened this issue Feb 20, 2024 · 4 comments
Closed

pubsub: With exactly once delivery and message ordering enabled, message order is not mantained #9440

MiquelPiza opened this issue Feb 20, 2024 · 4 comments
Assignees
@hongalex
Labels
api: pubsub Issues related to the Pub/Sub API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@MiquelPiza
Copy link

Client

PubSub

Expected behavior

Messages received are in the order the server delivers them

Actual behavior

Messages are reordered randomly when iterating over pendingMessages map

Additional context

On iterator.go, messageIterator.receive method, a pendingMessages map is created from the message list to remove any messages where modAck fails. After processing, a new message list is created iterating over the resulting pendingMessages object:

for _, m := range pendingMessages {
	v = append(v, m)
}
return v, nil

The order in which items are appended is not necessarily the same of the original list of messages (https://go.dev/blog/maps). Message ordering feature is thus broken
@MiquelPiza MiquelPiza added the triage me I really want to be triaged. label Feb 20, 2024
@product-auto-label product-auto-label bot added the api: pubsub Issues related to the Pub/Sub API. label Feb 20, 2024
@noahdietz noahdietz added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p3 Desirable enhancement or fix. May not be included in next release. and removed triage me I really want to be triaged. labels Feb 21, 2024
@noahdietz
Copy link
Contributor

triaged labels, feel free to change

@hongalex hongalex added priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. and removed priority: p3 Desirable enhancement or fix. May not be included in next release. labels Feb 21, 2024
@hongalex
Copy link
Member

I'm investigating this now. Probably requires keeping track of the messages in the order they came in a slice, and using that order to return them while keeping the pendingMessages map. I think the Python library might have a similar bug so following up there as well.

@hongalex hongalex changed the title Pubsub: With exactly once delivery and message ordering enabled, message order is not mantained pubsub: With exactly once delivery and message ordering enabled, message order is not mantained Feb 21, 2024
@hongalex
Copy link
Member

I was mistaken, the python client's behavior is different. When the receipt / lease modack happens (the first modack the client issues when a batch of messages are received), with exactly once delivery, the invalid ack_ids are kept track of in a separate array. When returning messages, the client iterates through the received messages and removes expired ack IDs.

I'll change the Go client to match this behavior.

@hongalex hongalex mentioned this issue Feb 26, 2024
Merged
@hongalex
Copy link
Member

Fix is released in cloud.google.com/go/[email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hongalex hongalex

Labels
api: pubsub Issues related to the Pub/Sub API. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@noahdietz @hongalex @MiquelPiza