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

pubsub: ackDeadline cancel context #11210

Closed
piapip opened this issue Dec 3, 2024 · 5 comments
Closed

pubsub: ackDeadline cancel context #11210

piapip opened this issue Dec 3, 2024 · 5 comments
Assignees
Labels
api: pubsub Issues related to the Pub/Sub API. status: investigating The issue is under investigation, which is determined to be non-trivial.

Comments

@piapip
Copy link

piapip commented Dec 3, 2024

Is your feature request related to a problem? Please describe.

Duplicated events when the process takes too long.

Describe the solution you'd like

Cancel the given context if ackDeadline has been reached.

Additional context

PubSub pushes the event back to the queue if it takes too long to process. I understand that it's for preventing event loss in case the application crashes. But if the actual event processing takes a long time to perform (says, the Database takes a long time to write), then it may create duplicated events.
Is it better to have the application handle the AckDeadline separately?
Or is it better to have the library cancel the given context?

@piapip piapip added the triage me I really want to be triaged. label Dec 3, 2024
@product-auto-label product-auto-label bot added the api: pubsub Issues related to the Pub/Sub API. label Dec 3, 2024
@hongalex hongalex added status: investigating The issue is under investigation, which is determined to be non-trivial. and removed triage me I really want to be triaged. labels Dec 3, 2024
@hongalex
Copy link
Member

hongalex commented Dec 3, 2024

By ackDeadline I'll assume you're referring to the message ack deadline that's set by the client library rather than the subscription's ackDeadline, which actually doesn't usually matter in most cases.

But if the actual event processing takes a long time to perform (says, the Database takes a long time to write), then it may create duplicated events.

This is true, but this is why we recommend tweaking MaxExtension (which is the total deadline, or how long a message deadline can be extended for before redelivery).

I'm not entirely sure why cancelling a context is relevant in the case of duplicated messages. Could you clarify more on that point?

@piapip
Copy link
Author

piapip commented Dec 4, 2024

I'll assume you're referring to the message ack deadline that's set by the client library rather than the subscription's ackDeadline

Yes, you are correct, I should have made it clearer that I refer to the client library.

Could you clarify more on that point?

My goal is to get a cue when the AckDeadline hits, so I can cancel the current process.
Maybe I can receive a cue if I set MaxExtension equal to AckDeadine, let me test that. << It doesn't mean much to have them equal, this just doubles the AckDeadline if I do so.

@piapip
Copy link
Author

piapip commented Dec 4, 2024

Oooh I found the kinda same issue here.
#5858

So mbrancato tried to handled context.Canceled from

	err = sub.Receive(givenCtx, func(ctx context.Context, m *pubsub.Message) {
		// Processing that may, at times, exceed the max extension time
                time.sleep(1 * time.Minute)
		m.Ack()
	})
	if err != context.Canceled {
		// TODO: Handle error.
	}

So I was thinking that maybe the sub.Receive would cancel the givenCtx when the MaxExtension has been reached. But it seems like it won't. After checking the library and your explanation, I'm quite sure that it won't.

Could you help me confirm it?

@hongalex
Copy link
Member

hongalex commented Dec 5, 2024

So this isn't exactly how contexts work. In the above example, givenCtx is something you as the developer would pass into the Receive function to tell the library to cancel execution. There is no way to signal that context is cancelled using givenCtx from the library's perspective.

My goal is to get a cue when the AckDeadline hits, so I can cancel the current process.

This isn't something the library supports currently as you already found out. I recommend creating a timer inside your handler function to check for how long the message has been processed for.

@piapip
Copy link
Author

piapip commented Dec 5, 2024

I agree. Thank you so much for the explanation!!!

@piapip piapip closed this as completed Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. status: investigating The issue is under investigation, which is determined to be non-trivial.
Projects
None yet
Development

No branches or pull requests

2 participants