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

Thread calling the Native API changed from main to background. #255

Merged
merged 2 commits into from
Aug 3, 2017

Conversation

dariuszseweryn
Copy link
Owner

Initially when working with the Android Bluetooth API (18 / Android 4.3) it was found that specific implementations (Samsung’s) cannot work correctly if called directly in callbacks from other calls. To mitigate the problem the next interaction was routed to Android main thread. Now when investigating the issue it turned out that it may be any other thread as long as it is not the callback thread.

Initially when working with the Android Bluetooth API (18 / Android 4.3) it was found that specific implementations (Samsung’s) cannot work correctly if called directly in callbacks from other calls. To mitigate the problem the next interaction was routed to Android main thread. Now when investigating the issue it turned out that it may be any other thread as long as it is not the callback thread.
@dariuszseweryn dariuszseweryn added this to the 1.4.0 milestone Jul 27, 2017
@dariuszseweryn dariuszseweryn self-assigned this Jul 27, 2017
@dariuszseweryn dariuszseweryn requested a review from uKL July 27, 2017 11:32
Copy link
Collaborator

@uKL uKL left a comment

Choose a reason for hiding this comment

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

I think that this change required a brief explanation in readme/changelog about your findings.

return new ClientComponentFinalizer() {
@Override
public void onFinalize() {
interactionExecutorService.shutdown();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually this is incorrect. Dagger will provide you a new instance of the executor each time you ask for it. You seems to be closing an instance that wasn't even started and you're leaving started service as is.

Provided executors should be scoped.

Copy link
Owner Author

Choose a reason for hiding this comment

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

From what I see the executors are scoped. And .shutdown() is executed in RxBleClient.finalize(). Or am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are correct, I missed the annotation @ClientScope

@dariuszseweryn
Copy link
Owner Author

Naturally this change will be described in the Changelog.

# Conflicts:
#	rxandroidble/src/main/java/com/polidea/rxandroidble/internal/connection/RxBleConnectionImpl.java
@dariuszseweryn dariuszseweryn merged commit 0d59132 into develop Aug 3, 2017
@dariuszseweryn dariuszseweryn deleted the topic/remove_main_thread branch August 3, 2017 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants