Ensure unsubscribing only removes a single subscription

[tomasz-tomczyk]

In our scheduling/calendar app, we subscribe for events in a given location, but we allow you to pass in a date filter, which we use in a custom resolver which allows us to limit what you receive to just the date you're looking. It looks something like this:

object :appointment_subscriptions do
  field :appointment_updated, :appointment do
    arg(:location_id, non_null(:id))
    arg(:start, :datetime)
    arg(:end, :datetime)

    config(fn args, _ctx} ->
        {:ok, topic: "appointments:#{args.location_id}"}
    end)

    resolve(fn appt, args, _res ->
      if is_nil(Map.get(args, :start)) or is_nil(Map.get(args, :end)) do
        {:ok, appt}
      else
        if Timex.between?(appt.start, args.start, args.end) do
          {:ok, appt}
        else
          {:error, nil}
        end
      end
    end)
  end
end

As you navigate between days in the calendar, our React state seems to start the new subscription before unsubscribing from the previous one.

The code added/modified here seems to then cause the Registry to get rid of both subscriptions and no more events are sent to the frontend.

I confirmed that by logging what is in the Registry at each step:

Registry keys after the first subscription:

[
"__absinthe__:doc:-576460752303396670:729765F61C701E8C26E2F1ED7A45018B6BEA1B82D5342A367150458036169796",
  {:appointment_updated, "appointments:12"},
]

Registry keys after the second subscription:

[
"__absinthe__:doc:-576460752303381886:E16BBF568DAC2E273B9721BC88B0F441164C82F77811FDCC2B7F19CB255DAB95",
  {:appointment_updated, "appointments:12"},
"__absinthe__:doc:-576460752303396670:729765F61C701E8C26E2F1ED7A45018B6BEA1B82D5342A367150458036169796",
  {:appointment_updated, "appointments:12"},
]

Registry keys after unsubscribing once:

[
"__absinthe__:doc:-576460752303381886:E16BBF568DAC2E273B9721BC88B0F441164C82F77811FDCC2B7F19CB255DAB95",
]

This looks like an invalid subscription since it no longer has the topic definition.

Just removing those 3 lines fixes the issues and I managed to replicate it in a test (omitted the custom resolver we have since it seems unnecessary for this example) and seemingly everything else passes, but I'm not entirely sure what the original code wanted to achieve.

Appreciate any feedback and guidance!

[benwilson512]

Hey @tomasz-tomczyk thanks or the PR. This is an area of the code base I always have to take time to re-remember every time I look at it :D.

Can you add some tests that verify a few things:

1. Publishing to topic 1 does not send messages after an unsubscribe.
2. Repeatedly subscribing and unsubscribing to the same topic does not grow the registry indefinitely.
3. Repeatedly subscribing and unsubscribing to different topics does not grow the registry indefinitely.

Do make sure to check the whole registry via Registry.select or similar. Registry has a few ets tables due to sharding so it's best to check it via its API.

[tomasz-tomczyk]

Thanks for your feedback!

1. Publishing to topic 1 does not send messages after an unsubscribe.

So with my test setup, they're using the same topic, just with added args that don't distinguish it as different topics.

I've added a test that confirms the unsubscribed topic did not receive a message in e64a6fa but:

• It's just one extra refute on top of the test I already had - worth combining to simplify?
• Did I understand what you expected in the first place?

2. Repeatedly subscribing and unsubscribing to the same topic does not grow the registry indefinitely.
3. Repeatedly subscribing and unsubscribing to different topics does not grow the registry indefinitely.

I've added tests in 55039ae but they're actually failing - the registry does grow at the moment, whether that piece of code in subscription.ex is there or not:

for field_key <- pdict_fields(doc_id) do
  Registry.unregister(registry, field_key)
end

Assertion failing:

     code:  assert [] == PubSub.list_registry_keys()
     left:  []
     right: [
              "__absinthe__:doc:-576460752303319933:439A5CADA422CD766FA8845A25B8BBBB5C97F523C30BEF654B0AB3030E802FDC",
              "__absinthe__:doc:-576460752303320253:439A5CADA422CD766FA8845A25B8BBBB5C97F523C30BEF654B0AB3030E802FDC",
              "__absinthe__:doc:-576460752303320573:439A5CADA422CD766FA8845A25B8BBBB5C97F523C30BEF654B0AB3030E802FDC"
            ]

Presumably, that's a bug with the original implementation too? Unless there's a reason these keys are kept? Happy to try to fix

[howleysv]

We have encountered this same bug, and implemented these changes to correct it.

@tomasz-tomczyk it'd be great if we could get these fixes into your PR and then get the issue fixed for good!

[howleysv]

We still want to clean up the record in the registry, but this only removes the entry associated with the doc_id and not all subscriptions with the same field_key

Suggested change

	for field_key <- pdict_fields(doc_id) do
	Registry.unregister(registry, field_key)
	end
	for field_key <- pdict_fields(doc_id) do
	Registry.unregister_match(registry, field_key, doc_id)
	end

[howleysv]

This is the registry name that is used to store the subscription entries internally

Suggested change

	Registry.keys(__MODULE__, self())
	Registry.keys(__MODULE__.Registry, self())

[tomasz-tomczyk]

Thanks @howleysv! That was very helpful.

@benwilson512 all tests pass now. Any other suggestions? 🙏 ❤️

[howleysv]

@tomasz-tomczyk Looks like the same fix was merged to main (unreleased) 6 months ago in #1336 and about to ship in 1.7.9