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

fix: Websocket error broadcast for unsubscribed ID #506

Merged
merged 2 commits into from
Oct 16, 2024

Conversation

calvincestari
Copy link
Member

@calvincestari calvincestari commented Oct 16, 2024

Closes apollographql/apollo-ios#2274
Closes apollographql/apollo-ios#2447

There is a sequence of events where the caller unsubscribes which removes their ID from subscribers but an incoming message is then received for that subscriber. It doesn't seem correct to broadcast that as an error to all remaining subscribers for unrelated messages.

Changes:

  • Only broadcast an error to all subscribers if there was no id field in the message
  • Do not send any error if the id was not found in subscribers.

@svc-apollo-docs
Copy link
Collaborator

svc-apollo-docs commented Oct 16, 2024

✅ Docs Preview Ready

No new or changed pages found.

Copy link

netlify bot commented Oct 16, 2024

Deploy Preview for eclectic-pie-88a2ba canceled.

Name Link
🔨 Latest commit e9ec645
🔍 Latest deploy log https://app.netlify.com/sites/eclectic-pie-88a2ba/deploys/670f27d975f0650008be15cb

Copy link

netlify bot commented Oct 16, 2024

Deploy Preview for apollo-ios-docc canceled.

Name Link
🔨 Latest commit e9ec645
🔍 Latest deploy log https://app.netlify.com/sites/apollo-ios-docc/deploys/670f27d96cdc0f0008bbf468

@@ -207,11 +218,6 @@ public class WebSocketTransport {
kind: .neitherErrorNorPayloadReceived)
responseHandler(.failure(websocketError))
}
} else {
Copy link
Member Author

Choose a reason for hiding this comment

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

An alternative approach to silencing this error path is to move unsubscribing IDs into another array that can be checked against if the ID is not found in subscribers. This would ensure we're discarding a message for an unsubscribing handler but I'm not sold on that being necessary.

The unsubscribing array should be of [String] type since there would be no need to store the handling closure again.

@calvincestari calvincestari changed the title fix: Websocket error broadcast for ID fix: Websocket error broadcast for unsubscribed ID Oct 16, 2024
@calvincestari calvincestari marked this pull request as ready for review October 16, 2024 18:07
Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@calvincestari calvincestari merged commit cdce4f9 into main Oct 16, 2024
39 checks passed
@calvincestari calvincestari deleted the fix/websocket-unknown-id-broadcast branch October 16, 2024 18:19
BobaFetters pushed a commit to apollographql/apollo-ios that referenced this pull request Oct 16, 2024
BobaFetters pushed a commit that referenced this pull request Oct 16, 2024
9d8aafbe fix: Websocket error broadcast for unsubscribed ID (#506)

git-subtree-dir: apollo-ios
git-subtree-split: 9d8aafbedece7483f397cb3a43f30f95892f840d
BobaFetters pushed a commit that referenced this pull request Oct 16, 2024
…bscribed ID

git-subtree-dir: apollo-ios
git-subtree-mainline: 05b5699
git-subtree-split: 9d8aafbedece7483f397cb3a43f30f95892f840d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants