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 serialization registry #398

Closed
wants to merge 1 commit into from

Conversation

sevdog
Copy link
Contributor

@sevdog sevdog commented Aug 21, 2024

This PR aims at two goals:

  1. Provide a common serialization/deserialization layer between RedisChannelLayer and PubSubRedisChannelLayer
  2. Provide a way to customize how messages are serialized before being forwarded to the broker (aka: redis), which may address problems like Django Channels Memory Leak on every message or connection channels#2094

The serializer registry is somehow inspired by that of django.core.serializers package.
This also prepares a future in which msgpack would no longer be a mandatory dependency.

@carltongibson
Copy link
Member

Oh, very exciting @sevdog — Thanks for thinking about this.

@sevdog
Copy link
Contributor Author

sevdog commented Aug 21, 2024

I would like to know your opinion @carltongibson regarding my design choice to put encryption into the serialization process.

Also I noted that currently thare are no tests for that feature, I could add them to this PR if you consider that useful.

@carltongibson
Copy link
Member

@sevdog Let me have a look/think. I've asked @bigfootjon and @acu192 to have a look too.

Copy link
Collaborator

@bigfootjon bigfootjon left a comment

Choose a reason for hiding this comment

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

I love the idea! I have a few questions and some requests but this is in great shape already!

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
channels_redis/pubsub.py Outdated Show resolved Hide resolved
channels_redis/serializers.py Show resolved Hide resolved
channels_redis/serializers.py Show resolved Hide resolved
tests/test_serializers.py Show resolved Hide resolved
README.rst Show resolved Hide resolved
channels_redis/serializers.py Outdated Show resolved Hide resolved
channels_redis/serializers.py Show resolved Hide resolved
channels_redis/core.py Outdated Show resolved Hide resolved
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.

This all looks quite reasonable.

I'm not quite seeing how it ties into the memory leak issue... 🤔 (Do we have an actual minimal reproduce for that anywhere?)

Comment on lines 119 to 121
issubclass(serializer_class, BaseMessageSerializer)
or hasattr(serializer_class, "serialize")
and hasattr(serializer_class, "deserialize")
Copy link
Member

Choose a reason for hiding this comment

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

... know the precedence of operators...

Shouldn't have people need to stop and think here. +1 to the extra parentheses.

Although... 🤔 Could we check isinstance on a Protocol here? (Perhaps not but...)

@sevdog
Copy link
Contributor Author

sevdog commented Sep 3, 2024

I belive the usage of Protocol could be added later, when a more extensive support for typing gets into the codebase.

@sevdog
Copy link
Contributor Author

sevdog commented Sep 3, 2024

I'm not quite seeing how it ties into the memory leak issue... 🤔 (Do we have an actual minimal reproduce for that anywhere?)

My hypothesis in django/channels#2094 (comment) was that the cause of the issue was in msgpack (I suppose hidden somewhere in the C/Cython module, which is used by default when compiled, there should also be some memory problem with the Python implementation).
From this my idea to add a mechanism which allow simple customization of the serialization phase of the channel layers (which is also available in other libraries which use a broker, ie: celery/kombu).
Here is the code used in my tests for django/channels#2094: https://github.com/sevdog/django-channels-memory-leak (I just added a JS to open multiple websockets to have the same amount of requests in each tests and to do not have to reproduce it by hand, also I dockerized the application. There are variuos config which I left commented for readability).

This mechanism can achieve multiple goals:

  • if there is a problem with a serializer you can switch to an other one (this can also be used for debugging purpose)
  • you may choose between space-efficient (msgpack) or readable (json) format
  • you may add a custom one without having to build a new channel-layer

@carltongibson
Copy link
Member

OK thanks @sevdog — let me have a look at that. I have no objection to this proposal. 👍

Copy link
Collaborator

@bigfootjon bigfootjon left a comment

Choose a reason for hiding this comment

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

Great work! Sorry it took me a bit to get back to this and review.

Just a few comments left, mostly just cleanup at this point

README.rst Outdated Show resolved Hide resolved
README.rst Outdated Show resolved Hide resolved
channels_redis/pubsub.py Outdated Show resolved Hide resolved
channels_redis/serializers.py Outdated Show resolved Hide resolved
@sevdog
Copy link
Contributor Author

sevdog commented Sep 20, 2024

Changes pushed.

Copy link
Collaborator

@bigfootjon bigfootjon left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for working through all my comments @sevdog

@carltongibson, when you get a chance could you do a pass on this PR and merge if you agree?

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.

This looks good. Sorry for the slow follow up, I missed your last ping @bigfootjon.

I just want to make a couple of tweaks to the README changes, which I'll do when I'm at the computer and than I'll pull this in.

Thanks @sevdog. Really nice addition.

@carltongibson
Copy link
Member

Will merge via #401.

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.

3 participants