When subscribed to a new_topic, send endpoints to all apps #70
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
karmanyaahm
changed the title
karmanyaahm
commented
Dec 17, 2023
Comment on lines
+261
to
+262
| io.heckel.ntfy.up.BroadcastReceiver.sendRegistration(baseContext, connectionId.baseUrl, topic) | ||
| // TODO is that the right context - looks like it works??? |
There was a problem hiding this comment.
Should that work? Or do I need to make a whole chain of contexts being passed down and stored? Not really sure what context is used for when creating the repository downstream. It seems to work as is.
karmanyaahm
commented
Dec 17, 2023
Comment on lines
96
to
101
| /* We don't send the endpoint here anymore, the foreground service will do that after | ||
| registering with the push server. This avoids a race condition where the application server | ||
| is rejected before ntfy even establishes that this topic exists. | ||
| This is fine from an application perspective, because other distributors can't even register | ||
| without a connection to the push server. | ||
| Unless the app sends registration twice. Then it'll get the endpoint.*/ |
There was a problem hiding this comment.
^ Pointing this out, hopefully is fine
There was a problem hiding this comment.
Actually, assuming the Android app is pushed out before the server changes, I'll have to leave the endpoint sending in for now. Otherwise, the new Android app will break for old servers.
karmanyaahm
commented
Dec 17, 2023
Comment on lines
+151
to
+168
|
|
||
| // TODO Where's the best place to put this function? This seems to be the only place | ||
| // with the access to the locks, but also globally accessible | ||
| // but also, broadcast receiver is for *receiving Android broadcasts* | ||
| public fun sendRegistration(context: Context, baseUrl : String, topic : String) { | ||
| val app = context.applicationContext as Application | ||
| val repository = app.repository | ||
| val distributor = Distributor(app) | ||
| GlobalScope.launch(Dispatchers.IO) { | ||
| // We're doing all of this inside a critical section, because of possible races. | ||
| // See https://github.com/binwiederhier/ntfy/issues/230 for details. | ||
|
|
||
| mutex.withLock { | ||
| val existingSubscription = repository.getSubscription(baseUrl, topic) ?: return@launch | ||
| val appId = existingSubscription.upAppId ?: return@launch | ||
| val connectorToken = existingSubscription.upConnectorToken ?: return@launch | ||
| val endpoint = topicUrlUp(existingSubscription.baseUrl, existingSubscription.topic) | ||
| distributor.sendEndpoint(appId, connectorToken, endpoint) |
There was a problem hiding this comment.
Where should I put this function?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Also needs cleaning and organization, but I'm opening this because I'm very excited to finally have something that works. So we previously discussed that I would do it using just timing data on the app because that would be simpler, but it really was not. Keeping track of the connection state was a whole thing and the worst thing is that it didn't solve the problem of losing the connection on server restart. This implementation only sends out new_endpoint after subscribing to the topic on the ntfy server.
Goes along with binwiederhier/ntfy#979