Skip to content

Support using premade websocket-rpc endpoint to create pubsub endpoint#97

Open
Taiwo-Sh wants to merge 1 commit intopermitio:masterfrom
Taiwo-Sh:master
Open

Support using premade websocket-rpc endpoint to create pubsub endpoint#97
Taiwo-Sh wants to merge 1 commit intopermitio:masterfrom
Taiwo-Sh:master

Conversation

@Taiwo-Sh
Copy link

@Taiwo-Sh Taiwo-Sh commented Feb 5, 2026

This PR addresses #96

WebsocketRPCEndpoint with appropriate methods type can now be used to create a PubSubEndpoint.

@orweis
Copy link
Contributor

orweis commented Feb 10, 2026

Hey @Taiwo-Sh, thanks for the contribution! The overall direction makes sense. I have some feedback after reviewing the code:

Suggestion: use **endpoint_kwargs instead of a premade instance

The main concern with accepting a pre-built WebsocketRPCEndpoint is that PubSubEndpoint needs to register its on_disconnect handler (for subscription cleanup), and the only way to do that on an existing instance is by mutating its private _on_disconnect / _on_connect lists. This creates tight coupling to RPC internals and introduces risk — e.g. the rpc_channel_get_remote_id value can get out of sync between the two objects.

A simpler approach that still solves #96: have PubSubEndpoint forward extra kwargs to WebsocketRPCEndpoint:

def __init__(
    self,
    methods_class=None,
    notifier=None,
    broadcaster=None,
    on_connect=None,
    on_disconnect=None,
    rpc_channel_get_remote_id=False,
    ignore_broadcaster_disconnected=True,
    **endpoint_kwargs,
):
    ...
    self.endpoint = WebsocketRPCEndpoint(
        self.methods,
        on_disconnect=[self.on_disconnect, *(on_disconnect or [])],
        on_connect=on_connect,
        rpc_channel_get_remote_id=rpc_channel_get_remote_id,
        **endpoint_kwargs,
    )

Then the usage from #96 becomes:

endpoint = PubSubEndpoint(
    methods_class=JobFetchRPCMethods,
    broadcaster=settings.REDIS_URL,
    serializing_socket_cls=JobFetchWebSocketProxy,
)

This avoids all the new branching/validation logic, private attribute access, and callback merging — PubSubEndpoint keeps full ownership of the endpoint lifecycle.

Other notes

  • The docstring for the endpoint param is truncated (ends with `met)
  • setup_server_with_no_methods_endpoint is identical to setup_server_with_premade_endpoint
  • test_premade_endpoint_custom_methods_preserved doesn't actually call or verify the custom method
  • test_premade_endpoint_with_methods_and_notifier doesn't verify the notifier identity is preserved

Happy to discuss — and thanks again for working on this!

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.

2 participants