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

Fix workers support when using Redis PubSub layer #298

Merged
merged 3 commits into from
Mar 8, 2022

Conversation

jalaziz
Copy link
Contributor

@jalaziz jalaziz commented Mar 6, 2022

The new Redis PubSub layer broke support for Channels workers. Add
support for workers by subscribing to non-owned channels instead of
throwing an exception.

fixes #270

@jalaziz
Copy link
Contributor Author

jalaziz commented Mar 6, 2022

While looking into this, I realized that the PubSub layer doesn't actually conform to the Channels Layer Spec.

In particular, the spec says:

Channels are a first-in, first out queue with at-most-once delivery semantics. They can have multiple writers and multiple readers; only a single reader should get each written message. Implementations must never deliver a message more than once or to more than one reader, and must drop messages if this is necessary to achieve this restriction.

While the PubSub layer is "at-most-once", it doesn't guarantee that a message will only be delivered to a single reader. This has implications for workers as having multiple workers listening to the same channel will result in duplicate messages.

A potential solution is to introduce a Redis Streams layer which allows such guarantees.

(filed #299 to track this)

@carltongibson
Copy link
Member

@jalaziz Thanks for this. Looks good. I rebased after #298.

We need to drop PY36. Time to do that anyway... Fancy making a PR for that?

@acu192
Copy link
Collaborator

acu192 commented Mar 7, 2022

This looks good to me. (Except the PY36 failure, but I agree with @carltongibson that we should drop PY36 in the next release.)

@jalaziz
Copy link
Contributor Author

jalaziz commented Mar 7, 2022

I've put up #301 to drop PY36.

@carltongibson
Copy link
Member

@jalaziz Good work. If you want to rebase here I'll take another look tomorrow 🛏️ 🎁

jalaziz and others added 3 commits March 7, 2022 11:48
The new Redis PubSub layer broke support for Channels workers. Add
support for workers by subscribing to non-owned channels instead of
throwing an exception.

fixes django#270
@jalaziz
Copy link
Contributor Author

jalaziz commented Mar 7, 2022

@jalaziz Good work. If you want to rebase here I'll take another look tomorrow 🛏️ 🎁

Was already on it :)

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Super, let's have it. Thanks (again) @jalaziz! 🏅

@carltongibson carltongibson merged commit 92b64a9 into django:main Mar 8, 2022
@acu192
Copy link
Collaborator

acu192 commented Mar 8, 2022

Thank you @jalaziz! Great work 👍, and super valuable.

@jalaziz
Copy link
Contributor Author

jalaziz commented Mar 8, 2022

Thank you @jalaziz! Great work 👍, and super valuable.

Happy to help 😃.

On a separate note, any chance this will be released soon? Hoping the pubsub layer will fix various issues we're having with dropped and misordered messages.

@carltongibson
Copy link
Member

I can roll a release this week I guess... — Anything else to clean up whilst we're here?

@jalaziz jalaziz deleted the 270 branch March 9, 2022 21:33
@jalaziz
Copy link
Contributor Author

jalaziz commented Mar 9, 2022

I can roll a release this week I guess... — Anything else to clean up whilst we're here?

There's the RuntimeError when the dictionary gets modified that we used to see, but that's not blocking anything. Not suer if there's anything that's easy to fix.

@carltongibson
Copy link
Member

3.4.0 is available on PyPI now https://pypi.org/project/channels-redis/ — Thanks all, and particularly @jalaziz on this one! 🎁

fredfsh pushed a commit to cobalt-robotics/channels_redis that referenced this pull request Jun 20, 2023
* Fix workers support when using Redis PubSub layer

The new Redis PubSub layer broke support for Channels workers. Add
support for workers by subscribing to non-owned channels instead of
throwing an exception.

Co-authored-by: Carlton Gibson <[email protected]>
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.

Channels' worker fails to start with PubSub layer
3 participants