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

Set SDK Initialization True before Completion Handler Invocation #761

Conversation

jsligh
Copy link
Collaborator

@jsligh jsligh commented Jun 5, 2024

Fixes #711

@jsligh jsligh marked this pull request as ready for review June 5, 2024 17:27
@jsligh jsligh self-assigned this Jun 5, 2024
Copy link
Contributor

@YuriyVelichkoPI YuriyVelichkoPI left a comment

Choose a reason for hiding this comment

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

There is a change in state management due to the misleading name of the flag.

The tasksCompletedSuccessfully indicates that tasks are just completed. SDK doesn't consider the status of tasks. So we should set

tasksCompletedSuccessfully = true; 

Otherwise, the function isSdkInitialized might return false if the publisher didn't provide a listner.

In any case, I'd suggest to fix the issue not by moving the place of changing value of the flag but calling the listener asynchronously.

@jsligh
Copy link
Collaborator Author

jsligh commented Jun 10, 2024

There is a change in state management due to the misleading name of the flag.

The tasksCompletedSuccessfully indicates that tasks are just completed. SDK doesn't consider the status of tasks. So we should set

tasksCompletedSuccessfully = true; 

Otherwise, the function isSdkInitialized might return false if the publisher didn't provide a listner.

In any case, I'd suggest to fix the issue not by moving the place of changing value of the flag but calling the listener asynchronously.

But calling it asynchronously would still not guarantee that isSdkInitialized returns true before the listener fires. It would just create a race condition would it not?

@YuriyVelichkoPI
Copy link
Contributor

YuriyVelichkoPI commented Jun 11, 2024

It shouldn't if to add the task (call the lister function) to the queue using, for example, Handler(Looper.getMainLooper())

…es after "tasksCompletedSuccessfully" is set to true.
@jsligh
Copy link
Collaborator Author

jsligh commented Jun 11, 2024

@YuriyVelichkoPI thank you for your review/help/direction, I have added the initializationListener to the main queue in another runnable so it is queued for execution AFTER tasksCompletedSuccessfully = true. I have tested this use case in the test app and the queue works as expected every time.

@YuriyVelichkoPI
Copy link
Contributor

YuriyVelichkoPI commented Jun 11, 2024

Oh, I missed the point that postOnMainThread is our internal function. The name mislead me to https://docs.mapbox.com/android/maps/api/10.18.1/mapbox-maps-android/com.mapbox.maps.threading/-animation-thread-controller/post-on-main-thread.html

Which says:

If called from Android Main thread - will be executed immediately in the same callchain otherwise will be posted on Android Main thread explicitly.

But in this case - it is just a wrapper for Handler(Looper.getMainLooper()), so it should work well!

Copy link
Contributor

@YuriyVelichkoPI YuriyVelichkoPI left a comment

Choose a reason for hiding this comment

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

LGTM

@jsligh jsligh merged commit 027fde2 into master Jun 11, 2024
5 checks passed
@jsligh jsligh deleted the 711-issdkinitialized-returns-false-in-sdkinitializationlistener-after-succeeded-initialization branch June 11, 2024 17:07
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.

isSdkInitialized returns false in SdkInitializationListener after succeeded initialization
2 participants