-
Notifications
You must be signed in to change notification settings - Fork 38.6k
Allow getting receipt from StompSession.Subscription.unsubscribe() #35224
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
Allow getting receipt from StompSession.Subscription.unsubscribe() #35224
Conversation
04bac3f
to
39760ed
Compare
Signed-off-by: Songdoeon <ehdjs9583@naver.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems reasonable to make this possible, but it could be an overloaded method, e.g. unsubsribeWithReceipt
to make it optional to get a receipt.
Hey, @rstoyanchev
What is the motivation for adding a new method instead of using the existing I think using the existing method makes more sense because it will align with other methods ( Also, according to the Lines 42 to 51 in 39760ed
And I see the implementation in this PR uses the same logic: Considering all of this I think it doesn't make sense to introduce a new overloaded method such as But I think the javadoc for |
Thanks for the review! I see two viable options here: A) keep the current change where both unsubscribe() overloads return Receiptable for consistency with send, subscribe, and acknowledge; callers can still ignore the return value, I initially went with A to keep the API symmetric with the other operations that already return Receiptable. In practice, applications don’t implement StompSession.Subscription and obtain it from the framework, so the impact should be limited. However, given Spring’s emphasis on strong backward compatibility in its public APIs , I’m happy to switch to B if you’d prefer the additive, non-breaking approach. |
Thanks for the feedback. I agree we should change the existing |
See gh-35224 Signed-off-by: Songdoeon <ehdjs9583@naver.com>
Motivation
Currently
Subscription.unsubscribe()
is avoid
method, so clients have no way to know when an UNSUBSCRIBE frame has actually been processed by the server. This PR bringsunsubscribe()
in line withsend()
andacknowledge()
by returning aReceiptable
that callers can use to register callbacks and track the receipt.Summary of Changes
Interface
StompSession.Subscription
— bothunsubscribe()
overloads now returnReceiptable
instead ofvoid
.Implementation
DefaultStompSession.unsubscribe(String, StompHeaders)
(private helper) now returns aReceiptable
by creating aReceiptHandler
after adding a receipt-id header.DefaultStompSession.DefaultSubscription.unsubscribe(...)
now invokes the helper and simply returns itsReceiptable
.Tests (in
DefaultStompSessionTests
)unsubscribeWithReceipt()
Verifies that calling
unsubscribe()
returns a non‑nullReceiptable
and that the sent frame still only contains the subscription ID when auto‑receipt is off.unsubscribeWithCustomHeaderAndReceipt()
Verifies that with
autoReceipt=true
and custom headers, the UNSUBSCRIBE frame contains the subscription ID, the custom header, and the generated receipt header, and that the returnedReceiptable
holds the same receipt-id.receiptReceivedOnUnsubscribe()
Verifies that if a RECEIPT frame arrives for the receipt-id, a task registered on the returned
Receiptable
is executed.All existing tests pass, and the new tests cover the added behavior.
Closes gh-22729