Skip to content

Conversation

sukunrt
Copy link
Member

@sukunrt sukunrt commented Oct 13, 2025

fixes: #611

@sukunrt sukunrt requested a review from MarcoPolo October 13, 2025 17:11
@sukunrt sukunrt changed the title cancell enqueued rpcs on receiving IDontWant cancel enqueued rpcs on receiving IDontWant Oct 13, 2025
@sukunrt sukunrt force-pushed the sukun/idontwant-cancel branch 3 times, most recently from 5ce8912 to 4f30121 Compare October 13, 2025 18:43
@sukunrt sukunrt force-pushed the sukun/idontwant-cancel branch from 4f30121 to 2ce4fe8 Compare October 13, 2025 20:30
Copy link
Contributor

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

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

some small changes, but I think we should merge this.

pubsub.go Outdated

messagesInNextRPC := 0
messageSlice := rpc.Publish
var msgIDSlice []string
Copy link
Contributor

Choose a reason for hiding this comment

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

reminder to run the fuzzer after this change

pubsub.go Outdated
if len(rpc.MsgIDs) == 0 {
msgIDSlice = make([]string, len(rpc.Publish))
} else {
panic("MsgIDs and Publish have different lengths")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is panic the best option? Is there a reason it's unsafe to use rpc.Publish as the source of truth?

Copy link
Member Author

Choose a reason for hiding this comment

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

This effectively says: panic if the MessageIDs are present but the length is < rpc.Publish length. I'm panicing here because it'd have paniced in the append later in the loop.

I'm changing this to work for any len of MessageIDs. Slightly ugly but also avoids allocation.

@sukunrt sukunrt force-pushed the sukun/idontwant-cancel branch 2 times, most recently from fc75da8 to 3476c19 Compare October 15, 2025 18:14
@sukunrt sukunrt force-pushed the sukun/idontwant-cancel branch from 3476c19 to 2926d76 Compare October 16, 2025 10:37
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.

Cancel queued Publish RPCs if IDONTWANT arrives in the meantime

2 participants