Skip to content

Every expired group will raise RuntimeError #18

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

Closed
nikita-davydov opened this issue Oct 28, 2019 · 6 comments
Closed

Every expired group will raise RuntimeError #18

nikita-davydov opened this issue Oct 28, 2019 · 6 comments

Comments

@nikita-davydov
Copy link

Screenshot 2019-10-28 at 18 41 57

This code will raise error every time if if channels[other_key] < now is True.
I fixed it by raise group_expiry param in the config.

@nikita-davydov
Copy link
Author

I have created pull request to fix it
#19

@adamhooper
Copy link
Contributor

Thank you for reporting this error -- and thank you for the pull request!

As documented in README.rst, you were probably right to increase group_expiry. The setting and default only exist because the Channel Layer Specification requires it.

After thinking on it for a while, I've developed the opinion that the spec is wrong. I've filed django/channels#1371 to nix group_expiry from the Channel Layer Specification. I don't see a good reason to expire connections.

... but let me ask you, @ndavydovdev: how did you encounter this bug? Do you actually want groups to expire? If so, why?

@nikita-davydov
Copy link
Author

nikita-davydov commented Oct 29, 2019

@adamhooper Thank you for your response! We have the system which manages user sessions and when the user stays in the one session(one group, one connection) a long time -> his group expires -> got this error. I think nobody uses standard group_expiry or people fix this error by increasing to one year as recommended. in fact, this code will never work properly and if somebody wants to use this group_expiry with very small error, this person will get this every time

@adamhooper
Copy link
Contributor

Awesome -- that's what I figured. Thanks for confirming.

I'll merge your pull request (and add a unit test). The expected result will be: the subscription disappears and nothing else breaks.

I'll also warn users on connect, if they're using the default value of group_expiry, to increase it. This advocacy may help change the specification.

@adamhooper
Copy link
Contributor

I still haven't added a warning. I was waiting on Django-channels devs for feedback on the spec. I still haven't heard back....

@adamhooper
Copy link
Contributor

... actually, come to think of it ... I'm going to disable this feature entirely. I don't think any user wants finite group_expiry. I consider it an error in the spec.

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

2 participants