Skip to content

A few tweaks to QueueBundle.Remove implementation#1240

Open
brandur wants to merge 1 commit intomasterfrom
brandur-queue-bundle-remove-improvements
Open

A few tweaks to QueueBundle.Remove implementation#1240
brandur wants to merge 1 commit intomasterfrom
brandur-queue-bundle-remove-improvements

Conversation

@brandur
Copy link
Copy Markdown
Contributor

@brandur brandur commented May 5, 2026

This builds on #1235 to bring in a few tweaks:

  • Removing a queue is a blocking operation because it needs to wait for
    the producer to finish up its jobs and shut down. It'd be better to
    provide a way for this not to block forever, so here we add a context
    parameter to QueueBundle.Remove similar to the one taken by
    Client.Stop. If the context becomes done before the producer
    resolves, QueueBundle.Remove falls through with the error.

  • Add a "stress" test case for QueueBundle.Remove. It's meant to
    detect a deadlock or other concurrency bug in case there is one and
    gives us a little more confidence that what we have here is right.

  • Renamed addProducer and removeProducer to producerAdd and
    producerRemove so they sort more nicely against each other.

  • Add changelog entry.

@brandur brandur force-pushed the brandur-queue-bundle-remove-improvements branch from 3f49ca4 to 6438bfc Compare May 5, 2026 11:25
Copy link
Copy Markdown
Contributor

@tigrato tigrato left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@brandur brandur force-pushed the brandur-queue-bundle-remove-improvements branch from 6438bfc to 276e05b Compare May 7, 2026 14:34
This builds on #1235 to bring in a few tweaks:

* Removing a queue is a blocking operation because it needs to wait for
  the producer to finish up its jobs and shut down. It'd be better to
  provide a way for this not to block forever, so here we add a context
  parameter to `QueueBundle.Remove` similar to the one taken by
  `Client.Stop`. If the context becomes done before the producer
  resolves, `QueueBundle.Remove` falls through with the error.

* Add a "stress" test case for `QueueBundle.Remove`. It's meant to
  detect a deadlock or other concurrency bug in case there is one and
  gives us a little more confidence that what we have here is right.

* Renamed `addProducer` and `removeProducer` to `producerAdd` and
  `producerRemove` so they sort more nicely against each other.

* Add changelog entry.
@brandur brandur force-pushed the brandur-queue-bundle-remove-improvements branch from 276e05b to 5f5a21c Compare May 7, 2026 14:34
@brandur brandur requested a review from bgentry May 7, 2026 22:38
@brandur
Copy link
Copy Markdown
Contributor Author

brandur commented May 7, 2026

@bgentry Ah doh, forgot to add you as a reviewer here. Seem okay to you?

Comment thread client.go
Comment on lines +2222 to +2231
shouldStop, stopped, finalizeStop := producer.StopInit()
if shouldStop {
select {
case <-ctx.Done():
finalizeStop(false)
return ctx.Err()
case <-stopped:
finalizeStop(true)
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not totally clear on some of the edge cases here. What happens if the producer is stopped, but the context also gets cancelled? Does the producer actually keep fully running, or do we go into an indeterminate state?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bgentry Hm, so StopInit will only return shouldStop = true if the producer is actually started. So in the case of a stopped producer, this block gets skipped, and the context is never checked. This shouldn't really happen though because the producer should get removed from c.producersByQueueName when it's stopped, so subsequent calls to Remove will return QueueNotFoundError. So I think not checking the cancelled context is probably okay here given it's an invalid call. Does that answer your question?

Copy link
Copy Markdown
Contributor

@bgentry bgentry left a comment

Choose a reason for hiding this comment

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

LGTM aside from the major question, I just want to make sure this is bulletproof and clearly documented.

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

Successfully merging this pull request may close these issues.

3 participants