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

HandlerThread for reconnecting AWSIoTMqttManager #619

Conversation

rvp-thunderbuild
Copy link
Contributor

Added a separate HandlerThread for reconnect attempts and publishing of queued messages.
This way the former doesn't need to happen on a newly created thread each time, and the latter no longer runs on the Main thread (since that is really not the best place).

This issue was mentioned before:
#532

@frankmuellr frankmuellr requested a review from scb01 December 17, 2018 19:43
@frankmuellr frankmuellr added iot Issues with the AWS Android SDK for Internet of Things (IoT) pull request labels Dec 17, 2018
@minbi
Copy link
Contributor

minbi commented Dec 27, 2018

Hi @rvp-thunderbuild ,

How does this pull request differ from #601 which creates and disposes of the handler thread when execution is completed?

@minbi minbi added the pending-community-response Issue is pending response from the issue requestor label Dec 27, 2018
@rvp-thunderbuild
Copy link
Contributor Author

Pull request #601 merely cleans up the handler thread after reconnecting. My proposed changes make use of a single handler thread instead of instantiating a new one each reconnect attempt. And also use this thread for publishing queued messages after a successful reconnect attempt instead of running these on the main thread.

@frankmuellr frankmuellr removed the pending-community-response Issue is pending response from the issue requestor label Jan 2, 2019
@mutablealligator
Copy link
Contributor

@rvp-thunderbuild Sorry for the delayed response. Can you rebase the changes with the current master and update the PR?

public void run() {
if (!mqttMessageQueue.isEmpty()) {
if (connectionState == MqttManagerConnectionState.Connected) {
if (mReconnectHandlerThread != null && mReconnectHandler != null) {
Copy link
Contributor

@mutablealligator mutablealligator Jan 21, 2019

Choose a reason for hiding this comment

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

I notice a difference here: The HandlerThread you created uses its looper whereas the existing code here uses the looper from the main thread. Do you have any reasoning behind this change?

Copy link
Contributor Author

@rvp-thunderbuild rvp-thunderbuild Jan 25, 2019

Choose a reason for hiding this comment

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

I don't believe that the main thread should be responsible for publishing messages from the Queue since this is network related. Originally the first message was published by the reconnecting thread, while all successive messages where published by the main thread. I changed this so that all queued message will be published by the reconnecting thread instead.

This makes more sense to me since either there is no connection and that thread is being used te reestablish it or there is a connection and that thread can be used to publish queued messages.

@palpatim palpatim removed the request for review from scb01 January 21, 2019 20:00
@palpatim palpatim assigned mutablealligator and unassigned scb01 Jan 21, 2019
# Conflicts:
#	aws-android-sdk-iot/src/main/java/com/amazonaws/mobileconnectors/iot/AWSIotMqttManager.java
@mutablealligator
Copy link
Contributor

@rvp-thunderbuild Sorry for the delayed response. Can you rebase the changes with the latest changes from master and remove the changes in .idea/ files?

@rvp-thunderbuild
Copy link
Contributor Author

@kvasukib I merged the master branch into this one and updated the .idea/ files

@mutablealligator
Copy link
Contributor

mutablealligator commented Feb 14, 2019

@rvp-thunderbuild Sorry for the delayed response. I see that the following 4 unit tests failed. Can you take a look at them if you have some cycles?

> Task :aws-android-sdk-iot:test

com.amazonaws.mobileconnectors.iot.AWSIotMqttManagerTest > testOfflinePublishQueueWithCallbacks FAILED
    java.lang.AssertionError at AWSIotMqttManagerTest.java:2148

com.amazonaws.mobileconnectors.iot.AWSIotMqttManagerTest > testDefaultDrainingInterval FAILED
    java.lang.AssertionError at AWSIotMqttManagerTest.java:2425

com.amazonaws.mobileconnectors.iot.AWSIotMqttManagerTest > testOfflinePublishQueueWithError FAILED
    java.lang.AssertionError at AWSIotMqttManagerTest.java:2532

com.amazonaws.mobileconnectors.iot.AWSIotMqttManagerTest > testLongerDrainingInterval FAILED
    java.lang.AssertionError at AWSIotMqttManagerTest.java:2468

The tests are located here: https://github.com/aws-amplify/aws-sdk-android/blob/master/aws-android-sdk-iot/src/test/java/com/amazonaws/mobileconnectors/iot/AWSIotMqttManagerTest.java

@rvp-thunderbuild
Copy link
Contributor Author

@kvasukib Those unit tests seem to fail because the OfflinePublishedQueue is no longer emptied from the main thread. Those tests are run with Robolectric instead of an actual device and any scheduled tasks are unable to run without specifically triggering these using Schedulers. In the current tests this is done for the main thread, however this no longer has any effect since it needs to be done for the new ReconnectHandlerThread.
I'm not a expert on Robolectric so I don't know if that is even possible without exposing the used HandlerThread to your unittests, which doesn't seem like a very good idea either.

@mutablealligator
Copy link
Contributor

@rvp-thunderbuild We will work on to update our unit tests to accommodate the changes you have made in this PR. Your effort is much appreciated!

@mutablealligator mutablealligator removed their assignment Dec 16, 2019
@jamesonwilliams jamesonwilliams force-pushed the master branch 4 times, most recently from 1008b5d to 13a2264 Compare April 14, 2020 00:04
@jamesonwilliams jamesonwilliams changed the base branch from master to develop June 17, 2020 18:24
@jamesonwilliams
Copy link
Contributor

This PR unfortunately has gone stale before it could be addressed. Will close for now. Please re-open if it can be updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iot Issues with the AWS Android SDK for Internet of Things (IoT)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants