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.

Sign up for GitHub

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

Jump to bottom

fix: awaitReconcile race with contextChanges #185

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: awaitReconcile race with contextChanges #185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions Confidence/api/Confidence.api
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ public final class com/spotify/confidence/Confidence : com/spotify/confidence/Co
public final fun activate ()V
public final fun apply (Ljava/lang/String;Ljava/lang/String;)V
public final fun asyncFetch ()V
public final fun awaitReconciliation (Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
public final fun awaitReconciliation (JLkotlin/coroutines/Continuation;)Ljava/lang/Object;
public static synthetic fun awaitReconciliation$default (Lcom/spotify/confidence/Confidence;JLkotlin/coroutines/Continuation;ILjava/lang/Object;)Ljava/lang/Object;
public final fun fetchAndActivate (Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
public fun flush ()V
public fun getContext ()Ljava/util/Map;
Expand Down Expand Up @@ -102,8 +103,8 @@ public final class com/spotify/confidence/ConfidenceError$ParseError : java/lang

public final class com/spotify/confidence/ConfidenceFactory {
public static final field INSTANCE Lcom/spotify/confidence/ConfidenceFactory;
public final fun create (Landroid/content/Context;Ljava/lang/String;Ljava/util/Map;Lcom/spotify/confidence/ConfidenceRegion;Lkotlinx/coroutines/CoroutineDispatcher;Lcom/spotify/confidence/LoggingLevel;)Lcom/spotify/confidence/Confidence;
public static synthetic fun create$default (Lcom/spotify/confidence/ConfidenceFactory;Landroid/content/Context;Ljava/lang/String;Ljava/util/Map;Lcom/spotify/confidence/ConfidenceRegion;Lkotlinx/coroutines/CoroutineDispatcher;Lcom/spotify/confidence/LoggingLevel;ILjava/lang/Object;)Lcom/spotify/confidence/Confidence;
public final fun create (Landroid/content/Context;Ljava/lang/String;Ljava/util/Map;Lcom/spotify/confidence/ConfidenceRegion;Lkotlinx/coroutines/CoroutineDispatcher;Lcom/spotify/confidence/LoggingLevel;J)Lcom/spotify/confidence/Confidence;
public static synthetic fun create$default (Lcom/spotify/confidence/ConfidenceFactory;Landroid/content/Context;Ljava/lang/String;Ljava/util/Map;Lcom/spotify/confidence/ConfidenceRegion;Lkotlinx/coroutines/CoroutineDispatcher;Lcom/spotify/confidence/LoggingLevel;JILjava/lang/Object;)Lcom/spotify/confidence/Confidence;
Comment on lines +106 to +107
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this part of the API change? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea 🤷

}

public final class com/spotify/confidence/ConfidenceFlagEvaluationKt {
Expand Down
24 changes: 20 additions & 4 deletions Confidence/src/main/java/com/spotify/confidence/Confidence.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,15 @@ import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.TimeoutCancellationException
import kotlinx.coroutines.cancel
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.distinctUntilChanged
import kotlinx.coroutines.flow.drop
import kotlinx.coroutines.launch
import kotlinx.coroutines.withTimeout
import kotlinx.serialization.Serializable
import kotlinx.serialization.json.Json
import kotlinx.serialization.json.encodeToJsonElement
Expand Down Expand Up @@ -77,10 +80,23 @@ class Confidence internal constructor(
return flagResolver.resolve(flags, getContext())
}

suspend fun awaitReconciliation() {
if (currentFetchJob != null) {
currentFetchJob?.join()
activate()
suspend fun awaitReconciliation(timeoutMillis: Long = 5000) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this support multiple concurrent calls? My concern is:
awaitReconciliation (1)
awaitReconciliation (2)
putContext -> this unlocks only (1)

try {
withTimeout(timeoutMillis) {
while (currentFetchJob == null ||
currentFetchJob?.isCompleted == true ||
currentFetchJob?.isCancelled == true
Comment on lines +87 to +88
Copy link
Member

Choose a reason for hiding this comment

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

Maybe replace these with currentFetchJob?.isActive != true

Copy link
Member Author

Choose a reason for hiding this comment

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

Mmm, perhaps. However I'm not certain having this is a great common timeout is great (imagine you call this function when we are actually NOT doing a network call, then it'll always just wait for timeoutMillis)... I might push an update.

Another approach would be to "delay" until cache.flagEvaluation.context == getContext() (but diffing them in some hashy way of course).

) {
// If this function is called just after a putContext it will likely just delay once.
delay(1)
Comment on lines +90 to +91
Copy link
Member

Choose a reason for hiding this comment

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

I think the un-predictability of when putContext is called + the short timespan of 1ms could make this code quite inefficient

Copy link
Member Author

Choose a reason for hiding this comment

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

Please explain more, not sure I understand the concern.

Copy link
Member

@fabriziodemaria fabriziodemaria Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think what I am getting at is: are there more idiomatic ways to achieve this observing behaviour? I am looking at job.invokeOnCompletion, for example, but I don't know if it does exactly what we need

}
if (currentFetchJob != null) {
currentFetchJob?.join()
activate()
}
}
} catch (e: TimeoutCancellationException) {
debugLogger?.logMessage("awaitReconciliation() timed out after $timeoutMillis")
}
}

Expand Down
Loading