update Subscribe/Unsubscribe handlers in ServerOpts #231
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
This change makes sense to me. @jba Do you see any problems with adding ServerSession to the Subscribe Handlers? |
Contributor
|
Agreed, this seems right. |
jba
previously approved these changes
Aug 4, 2025
Contributor
Author
|
@jba @samthanawalla sorry, just realized there was a ref to these two handlers in design/design.md so updated them as well. Since this would be a breaking change, do we have a change log that you want updated? |
Contributor
|
We don't need one; we run apidiff to compare the previous and candidate releases. |
jba
approved these changes
Aug 4, 2025
BC-ACherednichenko
pushed a commit
to BC-ACherednichenko/go-sdk
that referenced
this pull request
Aug 5, 2025
…ocol#231) ## Background Since the Server has a map of ServersSessions->subscriptions, and takes care of routing ResourceUpdate messages to ServerSessions, I think the decision was made to leave *ServerSession out of the SubscribeHandler/UnsubscribeHandler signatures. However, resource subscriptions are not specific to the a particular client and this information is kept private in the Session today. ## Proposal Align the Subscribe/Unsubscribe handlers with the other two handlers that are client specific (ProgressNotificationHandler, and RootListChangeHanlder), and include *ServerSession in the signature. It comes in practice because because a gateway server that needs to forward subscriptions still needs this information. The routing logic in Server is convenient and means that most servers won't need to know which ServerSessions are subscribed. But I think there are still use cases where the user will need to know.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
Since the Server has a map of ServersSessions->subscriptions, and takes care of routing ResourceUpdate messages to ServerSessions, I think the decision was made to leave *ServerSession out of the SubscribeHandler/UnsubscribeHandler signatures. However, resource subscriptions are not specific to the a particular client and this information is kept private in the Session today.
Proposal
Align the Subscribe/Unsubscribe handlers with the other two handlers that are client specific (ProgressNotificationHandler, and RootListChangeHanlder), and include *ServerSession in the signature.
It comes in practice because because a gateway server that needs to forward subscriptions still needs this information. The routing logic in Server is convenient and means that most servers won't need to know which ServerSessions are subscribed. But I think there are still use cases where the user will need to know.