Implement mergeable queue loop #265

Sakib25800 · 2025-04-02T15:49:11Z

Prerequisite to #247.

This PR implements the mergeable queue processing loop.

Pushes to the queue after pushes to the base branch
Pushes to the queue after PR edit

src/bors/handlers/pr_events.rs

src/bors/mergeable_queue.rs

src/bors/handlers/mod.rs

Sakib25800 · 2025-04-03T21:45:52Z

I've not actually written much (low level) async Rust code, so sorry for the dumb mistakes that this PR probably has.

Though, this has been the most fun PR to work on so far even with the skill issues 🫠.

Kobzol

This is a complex one! I left some comments.

src/database/client.rs

src/github/server.rs

src/bors/mergeable_queue.rs

Sakib25800 · 2025-04-12T19:15:11Z

Due to this PR being reworked a few times 😓, most of these comments should be resolved as they no longer apply.

Side note, I've got exams soon so will be slow to respond.

Sakib25800 · 2025-04-12T20:38:23Z

src/tests/mocks/bors.rs

@@ -616,6 +626,7 @@ impl BorsTester {
    async fn finish(self, bors: JoinHandle<()>) -> GitHubState {
        // Make sure that the event channel senders are closed
        drop(self.app);
+        self.mergeable_queue_tx.close();


I wanted mergeable_queue to behave like an mpsc channel, returning None when all senders are dropped.

But consume_mergeable_queue holds onto a sender so it can re‑enqueue items, thus keeping one sender reference alive indefinitely.

So I ended up opting for this explicit .close() approach (though I feel it doesn't look right).

If we used a normal tokio queue, we could use https://docs.rs/tokio/1.44.2/tokio/sync/mpsc/struct.Sender.html#method.downgrade. Since you already store Arc in the sender, I guess that you could emulate it with a "Weaksender" variant that only stores a Weak pointer.

Or, an alternative design could be that dequeue returns you both the item and a sender, and then before calling dequeue you drop the sender, so there should not be any outstanding senders.

I'll go for the Weaksender approach. I realised that the approach of checking if Arc::strong_count(&self.inner) == 2 is a little brittle.

Yeah that is a bit sketchy. I'll try to play around with it locally to see if we can figure out something better.

I would go with one of the following:

Store a weak sender Rc in the receiver. When a message is received, try to upgrade the weak to strong and return it together with the message. If it can't be upgraded, stop the reading loop. When a sender is dropped, it has to notify the reader, otherwise it might not ever realized that the last sender was removed.

Just add a message type to the reader that tells it to quit.

Honestly, I would just go with option 2. Dealing with the notify, RC counters, weak/strong references etc. seems like something that is too low-level for bors. We don't need high performance here, we just need to be sure that it is rock solid. With an explicit message type, that should be the easiest. We will need to make sure that the retry logic can deal with a failing send (in case the receiver has been stopped in the meantime), but that shouldn't be hard.

Option 2 sounds like what I had initially but with an explicit message type for shut down. I've now gone for this.

Option 1 looks quite complicated and not worth it considering shutdowns are only for tests.

Kobzol

The design with the closing boolean isn't super nice, and it's tricky to figure out all the possible interleavings for the dequeue method. An alternative could be to use weak senders, or create an explicit Stop message that can be sent to the queue to stop it. But even with the boolean I think that the implementation is fine, and it will run forever in the live service anyway, so it's not a real issue outside of tests.

src/bin/bors.rs

src/bors/handlers/pr_events.rs

src/database/operations.rs

Kobzol · 2025-04-15T14:15:09Z

src/tests/mocks/bors.rs

@@ -616,6 +626,7 @@ impl BorsTester {
    async fn finish(self, bors: JoinHandle<()>) -> GitHubState {
        // Make sure that the event channel senders are closed
        drop(self.app);
+        self.mergeable_queue_tx.close();


If we used a normal tokio queue, we could use https://docs.rs/tokio/1.44.2/tokio/sync/mpsc/struct.Sender.html#method.downgrade. Since you already store Arc in the sender, I guess that you could emulate it with a "Weaksender" variant that only stores a Weak pointer.

src/bors/mergeable_queue.rs

Kobzol · 2025-04-15T14:30:37Z

src/tests/mocks/bors.rs

@@ -616,6 +626,7 @@ impl BorsTester {
    async fn finish(self, bors: JoinHandle<()>) -> GitHubState {
        // Make sure that the event channel senders are closed
        drop(self.app);
+        self.mergeable_queue_tx.close();


Or, an alternative design could be that dequeue returns you both the item and a sender, and then before calling dequeue you drop the sender, so there should not be any outstanding senders.

Kobzol · 2025-04-23T07:35:12Z

Thanks! We don't actually need the closed boolean anymore if we consider the shutdown to be more of an "interrupt" that closes the queue only once (interrupts the reader who uses dequeue as an iterator), as that is how the queue is supposed to be used, but we can keep it, it doesn't really matter. If cou yould please squash the commits, and we can finally merge this :) Great work!

- Implement mergeable queue loop - Add to queue on PR edit base change - Add to queue on push to branch

Sakib25800 · 2025-04-23T08:28:47Z

Squashed.

Also removed the closed boolean.

Kobzol

Thank you for putting up with all my review comments 😆 Really awesome job.

Sakib25800 commented Apr 2, 2025

View reviewed changes

src/bors/handlers/pr_events.rs Show resolved Hide resolved

Kobzol reviewed Apr 3, 2025

View reviewed changes

src/bors/handlers/pr_events.rs Outdated Show resolved Hide resolved

src/bors/handlers/pr_events.rs Show resolved Hide resolved

src/bors/mergeable_queue.rs Outdated Show resolved Hide resolved

src/bors/handlers/mod.rs Outdated Show resolved Hide resolved

Sakib25800 force-pushed the add-mergeable-queue-loop branch from 76e3e15 to 0c58413 Compare April 3, 2025 18:26

Sakib25800 marked this pull request as ready for review April 3, 2025 21:43

Kobzol reviewed Apr 7, 2025

View reviewed changes

Sakib25800 force-pushed the add-mergeable-queue-loop branch from 4cb42d0 to 4bbb38c Compare April 12, 2025 19:12

Sakib25800 commented Apr 12, 2025

View reviewed changes

Kobzol reviewed Apr 15, 2025

View reviewed changes

Add mergeable queue

b4c28c3

- Implement mergeable queue loop - Add to queue on PR edit base change - Add to queue on push to branch

Sakib25800 force-pushed the add-mergeable-queue-loop branch from 6f0406b to b4c28c3 Compare April 23, 2025 08:27

Kobzol approved these changes Apr 23, 2025

View reviewed changes

Kobzol added this pull request to the merge queue Apr 23, 2025

Merged via the queue into rust-lang:main with commit c888eda Apr 23, 2025
2 checks passed

Implement mergeable queue loop #265

Implement mergeable queue loop #265

Uh oh!

Conversation

Sakib25800 commented Apr 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sakib25800 commented Apr 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kobzol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sakib25800 commented Apr 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sakib25800 Apr 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kobzol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kobzol commented Apr 23, 2025

Uh oh!

Sakib25800 commented Apr 23, 2025

Uh oh!

Kobzol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Sakib25800 commented Apr 2, 2025 •

edited

Loading

Sakib25800 commented Apr 3, 2025 •

edited

Loading

Sakib25800 commented Apr 12, 2025 •

edited

Loading

Sakib25800 Apr 12, 2025 •

edited

Loading