-
Notifications
You must be signed in to change notification settings - Fork 197
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
Conversation
b0173a2
to
0089029
Compare
Oh, very exciting @sevdog — Thanks for thinking about this. |
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. |
@sevdog Let me have a look/think. I've asked @bigfootjon and @acu192 to have a look too. |
There was a problem hiding this 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!
There was a problem hiding this 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?)
channels_redis/serializers.py
Outdated
issubclass(serializer_class, BaseMessageSerializer) | ||
or hasattr(serializer_class, "serialize") | ||
and hasattr(serializer_class, "deserialize") |
There was a problem hiding this comment.
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...)
0089029
to
162cc74
Compare
I belive the usage of |
162cc74
to
90ac7f0
Compare
My hypothesis in django/channels#2094 (comment) was that the cause of the issue was in This mechanism can achieve multiple goals:
|
OK thanks @sevdog — let me have a look at that. I have no objection to this proposal. 👍 |
There was a problem hiding this 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
90ac7f0
to
80fc30b
Compare
Changes pushed. |
There was a problem hiding this 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?
There was a problem hiding this 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.
Will merge via #401. |
This PR aims at two goals:
RedisChannelLayer
andPubSubRedisChannelLayer
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.