Skip to content
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

Add the option in ChannelsConsumer to confirm topic subscription #2799

Closed
1 of 3 tasks
moritz89 opened this issue Jun 1, 2023 · 7 comments
Closed
1 of 3 tasks

Add the option in ChannelsConsumer to confirm topic subscription #2799

moritz89 opened this issue Jun 1, 2023 · 7 comments

Comments

@moritz89
Copy link
Contributor

moritz89 commented Jun 1, 2023

In order to inform the subscriber (GraphQL client) that the GQL subscription has been activated, it would be useful to return a null message. In order to avoid a race condition, it would be useful to configure ChannelsConsumer.channel_listen to return None as soon as the subscription on the channels layer has been activated.

Feature Request Type

  • Core functionality
  • Alteration (enhancement/optimization) of existing feature(s)
  • New behavior

Description

In ChannelsConsumer.channel_listen, I would like to add the option of yielding None after the channel_layer.group_add loop:

# strawberry/channels/handlers/base.py:150
for group in groups:
    await self.channel_layer.group_add(group, self.channel_name)
    added_groups.append(group)

This would allow the subscriber to be written like this:

@strawberry.type
class FooSubscription:
    @strawberry.subscription
    @staticmethod
    async def foo_subscription(info: Info) -> AsyncGenerator[FooType | None, None]:
        # yield None
        async for message in info.context.ws.channel_listen("foo_type", groups=["a"]):
            if message is None:
                yield None
                continue
            yield FooType(message["payload"])

The commented out yield None is a workaround but confirms the subscription before the topic has been subscribed to. This is noticeable in tests when testing the Channels communication via REDIS.

Upvote & Fund

  • We're using Polar.sh so you can upvote and help fund this issue.
  • We receive the funding once the issue is completed & confirmed by you.
  • Thank you in advance for helping prioritize & fund our backlog.
Fund with Polar
@patrick91
Copy link
Member

@DoctorJohn what do you think? 😊

@DoctorJohn
Copy link
Member

DoctorJohn commented Jun 8, 2023

Interesting. Yielding None from channel_listen would certainly work, but I feel like it's not obvious to newcomers why the first yielded value would always be None. After a quick local experiement I think it's possible to make the channel_listen method return an AsyncGenerator after all groups have been added to the channel layer. The method could then be used like this:

@strawberry.type
class FooSubscription:
    @strawberry.subscription
    @staticmethod
    async def foo_subscription(info: Info) -> AsyncGenerator[FooType | None, None]:
        gen = await info.context.ws.channel_listen("foo_type", groups=["a"])
        yield None
        async for message in gen:
            yield FooType(message["payload"])

However, this would still be a breaking change since channel_listen would now return Awaitable[AsyncGenerator] instead of AsyncGenerator. User would have to migrate their code like this:

@strawberry.type
class FooSubscription:
    @strawberry.subscription
    @staticmethod
    async def foo_subscription(info: Info) -> AsyncGenerator[FooType | None, None]:
-        async for message in info.context.ws.channel_listen("foo_type", groups=["a"]):
+        async for message in await info.context.ws.channel_listen("foo_type", groups=["a"]):
            yield FooType(message["payload"])

@moritz89
Copy link
Contributor Author

moritz89 commented Jun 8, 2023

Thanks for investigating possible solutions. Would an idea be to have a separate method that returns the awaitable AsyncGenerator but is used by the original channel_listen method?

I'd agree that having the option of returning an awaitable AsyncGenerator instead yielding None as the first message is the more expected behavior.

@moritz89
Copy link
Contributor Author

Should I create a pull request based on the following interface or do have you already coded something up?

class ChannelsConsumer(AsyncConsumer):
    async def channel_listen(...) -> AsyncGenerator[Any, None]:
            return await self.base_channel_listen(...)

    async def base_channel_listen(...) -> Awaitable[AsyncGenerator[Any, None]]:
            ...

@DoctorJohn
Copy link
Member

Personally I would go with the breaking change of having only one method and changing the signature. But @patrick91 might prefer the backwards compatible change of introducing a separate method.

Should I create a pull request based on the following interface or do have you already coded something up?

I have a quick proof of concept without a test. So feel free to create a PR, I'll happily review it!

@patrick91
Copy link
Member

I think we can afford a breaking change for this 😊

@moritz89
Copy link
Contributor Author

moritz89 commented Jun 16, 2023

I created two approaches for handling channel_listen. One naive approach and one that uses a context manager to avoid missing cleanup.

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

No branches or pull requests

3 participants