-
Notifications
You must be signed in to change notification settings - Fork 529
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
Bugfix: multiple pushes per client for subscriptions that have a context_id
#1249
Bugfix: multiple pushes per client for subscriptions that have a context_id
#1249
Conversation
@@ -49,7 +49,7 @@ defmodule Absinthe.Execution.SubscriptionTest do | |||
@behaviour Absinthe.Subscription.Pubsub | |||
|
|||
def start_link() do | |||
Registry.start_link(keys: :unique, name: __MODULE__) | |||
Registry.start_link(keys: :duplicate, name: __MODULE__) |
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.
Had to change this to allow duplicated keys to make the test fail and also because the registry in the sub supervisor allows duplicated keys: https://github.com/absinthe-graphql/absinthe/blob/master/lib/absinthe/subscription/supervisor.ex#L37
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.
Yup, good call.
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.
Some minor things to tweak, but overall this looks good.
@@ -49,7 +49,7 @@ defmodule Absinthe.Execution.SubscriptionTest do | |||
@behaviour Absinthe.Subscription.Pubsub | |||
|
|||
def start_link() do | |||
Registry.start_link(keys: :unique, name: __MODULE__) | |||
Registry.start_link(keys: :duplicate, name: __MODULE__) |
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.
Yup, good call.
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.
Fantastic, thanks for tracking this down!
Amazing, thank you! |
This PR aims to close this issue: #1064
Originally this issue seems to be introduced by this PR -- because of this specific change. It was introduced in the version
v1.6.0
On the
v1.5.5
the deduplication happened usingMap.new
when looking at the Registry. By checking the current version, using the same strategy seems to fix the problem since the documents should be identical.