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: awaitReconcile race with contextChanges #185

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

Conversation

nicklasl
Copy link
Member

There was a case for race conditions that could occur when calling putContext() followed by a awaitReconciliation() where the contextChanges updates did not trigger a fetchAndActivate before awaitReconciliation had finished.

@nicklasl nicklasl changed the title fix: fix awaitReconcile race with contextChanges fix: awaitReconcile race with contextChanges Nov 22, 2024
@nicklasl nicklasl marked this pull request as ready for review November 22, 2024 11:00
Comment on lines +106 to +107
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;
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 🤷

Copy link
Member

@fabriziodemaria fabriziodemaria left a comment

Choose a reason for hiding this comment

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

Added a few comments about various corner cases that I would like to discuss a bit further

Comment on lines +90 to +91
// If this function is called just after a putContext it will likely just delay once.
delay(1)
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

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

Comment on lines +87 to +88
currentFetchJob?.isCompleted == true ||
currentFetchJob?.isCancelled == true
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 (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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants