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

No updates after subscribing to a broadcast that "doesn't exist yet" #59

Closed
glasserc opened this issue Aug 7, 2018 · 6 comments
Closed
Assignees
Labels
bug Something isn't working p1

Comments

@glasserc
Copy link

glasserc commented Aug 7, 2018

To reproduce:

  • Using wscat, issue a message such as: {"messageType": "broadcast_subscribe", "broadcasts": {"remote-settings/blahblahblah": "abcd"}}.
  • No "broadcasts" are received (because the broadcast doesn't exist yet).
  • Post an update to this broadcast ID using something like curl -X PUT -H "Authorization: Bearer XXX" -d "BREAKING NEWS: v4" https://megaphone-default.stage.mozaws.net/v1/broadcasts/remote-settings/blahblahblah.

Expected behavior:

  • wscat eventually receives an update saying that the blahblahblah subscription has a new version.

Actual behavior:

  • wscat only receives boring pings for a while (until I got bored).

@pjenvey observes that this bug may also affect any subscriptions enumerated as part of the "hello" message.

@bbangert bbangert self-assigned this Aug 21, 2018
@bbangert bbangert added in progress bug Something isn't working p1 labels Aug 21, 2018
@bbangert
Copy link
Member

This is intentional behavior, we can't have clients subscribing to broadcasts that we don't know exist.

Implementation-wise, to be as memory-efficient as possible for a client, each broadcast they're interested in is matched to the broadcast id in our system, and we store a list of those id's for the client. If we do not know of the broadcast id the client is interested in, we have nothing we can store. If we let a client force us to add data to our broadcast id list so we can store the id, we've opened ourselves to an attack vector.

Sorry about the delay in getting back to this.

@glasserc
Copy link
Author

This is intentional behavior, we can't have clients subscribing to broadcasts that we don't know exist.

If that's the case, shouldn't we give them an error when they try, so that they know something went wrong? Currently we just sort of "fail silently".

@glasserc glasserc reopened this Aug 23, 2018
@bbangert
Copy link
Member

We could return an error in hello, but we'd have to do it in a not-very-error-like way to avoid causing errors with old clients. I'm not entirely sure what the client would do when seeing an error, besides for logging it. Ideally we don't ship Firefox components with broadcaster id's that we haven't fully implemented and propagated on the server-side.

Would the client use this purely for logging? We could add a new broadcast_error field that the old clients should ignore that the client can log.

@glasserc
Copy link
Author

I'm mostly concerned about manual ad-hoc testing as might be done by QA or as part of development. No response at all indicates that the broadcast ID is correct, which it isn't, and that its current version is the most up-to-date, which we can't know. Can we respond with something like "broadcasts": {"some-unknown-id": "ERROR: unknown broadcast id"} ?

@bbangert
Copy link
Member

Won't the client store that as the version in that case? I suppose that might be ok. Alternatively, maybe we have a "errors" key inside the "broadcasts" hash with a hash of failed broadcast-ids?

Ie:

"broadcasts": {"errors": {"some-unknown-id": "Unknown broadcast id"}}

afaik, the current client will ignore the errors key, and QA can see the issue. The client could in the future support that somehow as well.

@glasserc
Copy link
Author

That sounds fine to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p1
Projects
None yet
Development

No branches or pull requests

2 participants