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

Reimplement channel_listen as context manager #2856

Merged
merged 16 commits into from
Jul 4, 2023

Conversation

moritz89
Copy link
Contributor

@moritz89 moritz89 commented Jun 16, 2023

Why:

  • Allow user code to be run after Channels subscription
  • Avoids race condition when confirming GQL subscriptions

This change addresses the need by:

  • Returning the async generator instead of runnning it

Description

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@moritz89
Copy link
Contributor Author

Addresses issue #2799

@botberry
Copy link
Member

botberry commented Jun 16, 2023

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


This release updates the API to listen to Django Channels to avoid race conditions
when confirming GraphQL subscriptions.

Deprecations:

This release contains a deprecation for the Channels integration. The channel_listen
method will be replaced with an async context manager that returns an awaitable
AsyncGenerator. This method is called listen_to_channel.

An example of migrating existing code is given below:

# Existing code
@strawberry.type
class MyDataType:
   name: str

@strawberry.type
class Subscription:
   @strawberry.subscription
   async def my_data_subscription(
      self, info: Info, groups: list[str]
   ) -> AsyncGenerator[MyDataType | None, None]:
      yield None
      async for message in info.context["ws"].channel_listen("my_data", groups=groups):
         yield MyDataType(name=message["payload"])
# New code
@strawberry.type
class Subscription:
   @strawberry.subscription
   async def my_data_subscription(
      self, info: Info, groups: list[str]
   ) -> AsyncGenerator[MyDataType | None, None]:
      async with info.context["ws"].listen_to_channel("my_data", groups=groups) as cm:
         yield None
         async for message in cm:
            yield MyDataType(name=message["payload"])

Here's the preview release card for twitter:

Here's the tweet text:

🆕 Release (next) is out! Thanks to @moritz89 for the PR 👏

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

@moritz89
Copy link
Contributor Author

This is the simplest implementation possibility. However, it has the problem that a leak can occur if the async generator is never started. Will create a second approach that uses a context manager to ensure that the cleanup will be done.

@moritz89 moritz89 marked this pull request as draft June 16, 2023 10:21
@moritz89
Copy link
Contributor Author

Added a second option of starting channel_listen with a context manager to ensure that group_discard is called after it is started.

@moritz89
Copy link
Contributor Author

@DoctorJohn @patrick91 Wanted to ask which approach you'd prefer? ContextManager or the simple approach

@DoctorJohn
Copy link
Member

@DoctorJohn @patrick91 Wanted to ask which approach you'd prefer? ContextManager or the simple approach

Hi @moritz89 thanks for the PR. I prefer the ContextManager approach. It's the one I did my quick proof of concept with. Main argument for it is the cleanup ability you already mentionened in your comment. Feel free to remove the "simple approach".

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

Merging #2856 (1a15f39) into main (5761710) will decrease coverage by 0.18%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2856      +/-   ##
==========================================
- Coverage   96.36%   96.18%   -0.18%     
==========================================
  Files         207      211       +4     
  Lines        8948     9212     +264     
  Branches     1645     1701      +56     
==========================================
+ Hits         8623     8861     +238     
- Misses        206      226      +20     
- Partials      119      125       +6     

@DoctorJohn DoctorJohn self-requested a review June 22, 2023 00:04
Why:

- Allow user code to be run after Channels subscription
- Avoids race condition when confirming GQL subscriptions

This change addresses the need by:

- Returning the async generator instead of runnning it
Why:

- Ensure no leak of group_add / group_discard

This change addresses the need by:

- Encapsulating channel_listen with a context manager
Why:

- Only support code path without resource leak

This change addresses the need by:

- Removing naive channel_listen methods
@moritz89 moritz89 marked this pull request as ready for review June 22, 2023 14:18
@moritz89
Copy link
Contributor Author

Isn't this fun, adding breaking changes 😅 Anything else that should be updated?

@patrick91
Copy link
Member

@moritz89 it is a lot of work to make it backwards compatibile with a deprecation? 😊

@moritz89
Copy link
Contributor Author

@moritz89 it is a lot of work to make it backwards compatibile with a deprecation? blush

Not at all. The question is only how this should be handled. It would result in two functions in the API and the question is how they should be named. One option is to append _ctx to the new function.

@moritz89
Copy link
Contributor Author

moritz89 commented Jun 24, 2023

@patrick91 I've had a look at unifying the calls in the following style, but also that did not work and going down that line of thought makes the library code itself more and more tricky. If you have any suggestions, would be glad to take them up.

class ChannelListen:
    def __init__(
        self, type: str, *, timeout: Optional[float] = None, groups: Sequence[str] = ()
    ) -> None:
        ...

    async def __call__(
        self, type: str, *, timeout: Optional[float] = None, groups: Sequence[str] = ()
    ) -> Any:
        ...

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

    async def __aexit__(self) -> None:
        ...

    async def _channel_listen_generator(
        self, queue: asyncio.Queue, timeout: Optional[float]
    ) -> AsyncGenerator[Any, None]:
        ...

class ChannelsConsumer(AsyncConsumer):
    ...
    channel_listen = ChannelListen

Why:

- Ensure context manager channel_listen works

This change addresses the need by:

- Updating existing tests
- Adding new test to show yield after channel subscription
@moritz89
Copy link
Contributor Author

moritz89 commented Jun 24, 2023

Fixed mypy issues and updated existing tests and added a new one, but don't know why one is failing:

ERROR 😡 tests/cli/test_server.py::test_debug_server_routes - KeyError: 'STRAWBERRY_DEBUG_SERVER_SCHEMA'

Would appreciate help in fixing it

EDIT: Seems to be passing in the CI 😅

@moritz89 moritz89 changed the title Add simple awaitable channel_listen Reimplement channel_listen as context manager Jun 24, 2023
@DoctorJohn
Copy link
Member

Methods don't know whether they're called with or without await. To keep some kind of backwards compatibility we would either need:

  1. two methods (the old one returning AsyncGenerator and the newer one Awaitable[AsyncGenerator])
  2. or a parameter which decides whether to return AsyncGenerator or Awaitable[AsyncGenerator]

I would recommend using two methods and naming the newer one something like listen_to_channel. Do you think that's a good name @moritz89 ?

Using the second approach would be akward IMHO since it would require a second migration after we end the deprecation period and remove the parameter deciding the return type.

@moritz89
Copy link
Contributor Author

@DoctorJohn If the plan is to deprecate the existing approach, then listen_to_channel sounds good. If both approaches should be kept, then the names should only be differentiated by a perfix/suffix.

I'll create a commit assuming you meant the former (deprecate the existing approach).

@moritz89
Copy link
Contributor Author

How should the deprecation be communicated to the library users?

@DoctorJohn
Copy link
Member

I'll create a commit assuming you meant the former (deprecate the existing approach).

Yup, that's what I meant. I think the new approach is superior and covers all usecases of the old one.

How should the deprecation be communicated to the library users?

Using the warnings module, just like here for example:

warnings.warn(
"`info.field_nodes` is deprecated, use `selected_fields` instead",
DeprecationWarning,
stacklevel=2,
)

@moritz89
Copy link
Contributor Author

@patrick91 The MR won't break compatibility but adds a deprecation warning

docs/integrations/channels.md Outdated Show resolved Hide resolved
docs/integrations/channels.md Outdated Show resolved Hide resolved
@patrick91
Copy link
Member

@patrick91 The MR won't break compatibility but adds a deprecation warning

Thank you so much! <3

moritz89 and others added 4 commits June 27, 2023 19:38
Co-authored-by: Jonathan Ehwald <github@ehwald.info>
Co-authored-by: Jonathan Ehwald <github@ehwald.info>
Downgraded from breaking change to deprecation
@moritz89
Copy link
Contributor Author

moritz89 commented Jul 3, 2023

@DoctorJohn @patrick91 Does this MR require anything else or can it be merged?

Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

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

sorry for the late review, it looks good to me! I only left a minor comment on the release note, maybe we can simplify the snippet there?

async def my_data_subscription(
self, info: Info, groups: list[str]
) -> AsyncGenerator[MyDataType | None, None]:
yield None
Copy link
Member

Choose a reason for hiding this comment

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

is there any reason why there's this yield none here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to show how GQL subscription confirmations (with null data) should be migrated, i.e., that confirmation messages should be sent before the async for loop. However, for every else not confirming their GQL subscriptions the yield None can be omitted in both new and old code variants.

How should the doc be updated to reflect that?

Copy link
Member

Choose a reason for hiding this comment

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

oh I see, let's leave it there.

For the docs, maybe we could add a paragraph about confirming subscriptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Added a section in the channels.md doc

strawberry/channels/handlers/base.py Show resolved Hide resolved
@patrick91 patrick91 dismissed DoctorJohn’s stale review July 4, 2023 14:03

all done now 😊

@patrick91 patrick91 merged commit c18be1f into strawberry-graphql:main Jul 4, 2023
38 of 39 checks passed
@botberry
Copy link
Member

botberry commented Jul 4, 2023

Thanks for contributing to Strawberry! 🎉 You've been invited to join
the Strawberry GraphQL organisation 😊

You can also request a free sticker by filling this form: https://forms.gle/dmnfQUPoY5gZbVT67

And don't forget to join our discord server: https://strawberry.rocks/discord 🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants