Splits the SessionDataService into a service and client #5403
Conversation
Generated by 🚫 Danger |
Coverage Report 1Affected Products
Test Logs |
|
The public api surface has changed for the subproject firebase-sessions: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
Size Report 1Affected Products
Test Logs |
|
|
||
| private var service: Messenger? = null | ||
| private var serviceBound: Boolean = false | ||
| private val queuedMessages = LinkedList<Message>() |
There was a problem hiding this comment.
does this need to be thread-safe?
There was a problem hiding this comment.
Updated to use LinkedBlockingDeque
| * whether or not this foregrounding event should result in a new session being generated. | ||
| */ | ||
| fun foregrounded(activity: Activity): Unit { | ||
| sendLifecycleEvent(SessionLifecycleService.FOREGROUNDED) |
There was a problem hiding this comment.
I'm almost certain this isn't thread-safe.
There was a problem hiding this comment.
Updated to use LinkedBlockingDeque
| * Queues the given [Message] up for delivery to the [SessionLifecycleService] once the connection | ||
| * is established. | ||
| */ | ||
| private fun queueMessage(msg: Message) { |
There was a problem hiding this comment.
This isn't thread-safe and I think it needs to be. Removing the oldest messages is certainly the most correct thing to do, but I think it isn't terrible to simply drop the current message in this case. If you're comfortable with this, then usingLinkedBlockingQueue::offer would work beautifully here
There was a problem hiding this comment.
Updated to use LinkedBlockingDeque. I still try to shrink the queue before calling ::offer. It's still possible the offer fails if some other thread adds the latest, but IMO it's still worth try to clean the oldest first.
| private fun sendLifecycleEvent(msg: Message) { | ||
| if (service != null) { | ||
| try { | ||
| service?.send(msg) |
There was a problem hiding this comment.
If this succeeds, then why not drain the queue here as well?
There was a problem hiding this comment.
Updated to always try to drain the latest messages on send.
…es and only sends the latest BACKGROUND and FOREGROUND messages to the server when draining. Also draining the queue on every send in the event of a previous failure.
|
The public api surface has changed for the subproject firebase-sessions: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
| */ | ||
| private fun sendLifecycleEvent(messageCode: Int) { | ||
| val allMessages = ArrayList<Message>() | ||
| queuedMessages.drainTo(allMessages) |
There was a problem hiding this comment.
Maybe this line and the line above could be extracted to a method drainQueuedToNewList()? You do it a couple of times.
| */ | ||
| private fun queueMessage(msg: Message) { | ||
| Log.i(TAG, "Queueing message ${msg.what}") | ||
| while (queuedMessages.size >= MAX_QUEUED_MESSAGES) { |
There was a problem hiding this comment.
This isn't really thread-safe (two threads could be doing this at the same time with unpredictable results). Though it's probably okay, we can also probably live without it.
| // Note: it is possible this still fails to insert if another thread inserts in the meantime. | ||
| // However this indicates that many lifecycle events are ocurring and so a missed message is | ||
| // very unlikely to result in a missed session. | ||
| queuedMessages.offer(msg) |
There was a problem hiding this comment.
maybe log the result (true if success, false if full)?
|
|
||
| private val boundClients = mutableListOf<Messenger>() | ||
| private var curSessionId: String? = null | ||
| private var isAppInForeground: Boolean = false |
There was a problem hiding this comment.
Didn't we agree that this could be dropped?
|
The public api surface has changed for the subproject firebase-sessions: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
|
The public api surface has changed for the subproject firebase-sessions: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
…ver and refactors queueing in client.
| } | ||
| ) | ||
| } catch (e: Exception) { | ||
| if (e is DeadObjectException) { |
There was a problem hiding this comment.
Why not catch DeadObjectException and then catch Exception ?
|
The public api surface has changed for the subproject firebase-sessions: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
|
The public api surface has changed for the subproject firebase-sessions: Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly. |
4 participants
Created a SessionLifecycleService class which contains all the service-side code, and a SessionLifecycleClient object which handles the connection to the server and the sending and receiving of events.
Also added documentation to the code.