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

Bugfix: multiple pushes per client for subscriptions that have a context_id #1249

Conversation

beligante
Copy link
Contributor

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 using Map.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.

@@ -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__)
Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, good call.

Copy link
Contributor

@benwilson512 benwilson512 left a 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__)
Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, good call.

test/absinthe/execution/subscription_test.exs Outdated Show resolved Hide resolved
lib/absinthe/subscription.ex Outdated Show resolved Hide resolved
Copy link
Contributor

@benwilson512 benwilson512 left a 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!

@benwilson512 benwilson512 merged commit 86e6c80 into absinthe-graphql:master Jun 30, 2023
7 checks passed
@jonatanklosko
Copy link
Contributor

Amazing, thank you!

@beligante beligante deleted the fix-multiple-pushes-to-same-context-id branch October 5, 2023 11:21
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.

4 participants