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 task interference bugs and task repeating #171

Merged
merged 2 commits into from
Nov 2, 2023

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Nov 1, 2023

  • Simplifies how we use task manager to only use single enqueuing of a task, and rely on Kolibri's task requeuing within the life cycle of a task to handle repeat behaviour
  • Updates to always use APPEND_OR_REPLACE to ensure that tasks append while running or replace if not running.
  • Updates Python for Android to a version on our fork that allow defining a main function in the specified module that will be executed if it exists - it is passed what would normally be passed as the environment variable argument as its sole argument
  • This helps prevent the environment variable getting set to the incorrect value when multiple threads are operating (as they do in the work manager context) - this means that job execution will always be passed the proper job_id, and not accidentally read the incorrect job_id from either from an environment variable or a variable set on the __main__ module that is shared across threads.

I tested this with a dummy task defined in the android_app_plugin that used the retry_in method of the job object to repeat itself every 5 seconds. It did this successfully, and also did not interfere or attempt to run other currently active tasks (such as the ping back task or the network connection checking task) which I had seen in previous testing.

Full diff for the change in our P4A fork here: learningequality/python-for-android@4a3c74c...5eff8c9

…ion.

Create main function in taskworker to allow arguments to be passed in a way
that prevents confusion across threads.
@pcenov
Copy link
Member

pcenov commented Nov 2, 2023

Hi @rtibbles syncing is working correctly now and the only potential issue that I was able to identify is that the sync status displayed at Coach > Class home > View learners never changes from 'Synced' to 'Not connected to server' (after having force stopped the app) - had to wait for about 20 minutes for it to change to 'Not recently synced or unable to sync'.

There are some errors in the logs such as:

ERROR 2023-11-02 13:17:57,394 kolibri.core.device.soud [user=2389bbe0d3cf341764625349a5c594e8] [server=6a40d55608b125e4a9e45fb854476922] Critical error occurred during syncing: Stage `cleanup` failed
ERROR 2023-11-02 13:17:57,466 kolibri.core.device.soud Stage `cleanup` failed

so adding the logs for you to review as well:

Android device:
android.zip
ubuntu_server.zip

@rtibbles
Copy link
Member Author

rtibbles commented Nov 2, 2023

Thanks @pcenov - looking at the logs, I don't think that's the result of this work, but I'll ask around to see!

@rtibbles
Copy link
Member Author

rtibbles commented Nov 2, 2023

Yes, have filed a follow up on Kolibri: learningequality/kolibri#11485 - so I think this PR is doing what it ought!

@pcenov
Copy link
Member

pcenov commented Nov 2, 2023

@rtibbles it seems that there's still something wrong with the app though. Now it has stopped syncing (for a brand new learner user) so once again adding the logs.

The server is the same as before:

Android2.zip

ubuntu_server2.zip

Copy link
Member

@jredrejo jredrejo left a comment

Choose a reason for hiding this comment

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

Tested and syncing task is working well, even using an user migrated from a tablet to the server.
For the code I'm not totally sure because I don't know completely the inner details of the task system in Android but https://developer.android.com/reference/androidx/work/WorkManager#enqueueUniqueWork(java.lang.String,androidx.work.ExistingWorkPolicy,java.util.List%3Candroidx.work.OneTimeWorkRequest%3E) seems to be done exactly for the purpose this PR is using it so it looks ok.

On the other hand I've seen several error messages in the server. They don't look related to this PR but I'll follow them on a separate thread/issue.

@rtibbles
Copy link
Member Author

rtibbles commented Nov 2, 2023

@pcenov looking at the logs, this seems unrelated to Android specifically, so I have filed the error I saw in the logs as a follow up issue here: learningequality/kolibri#11488

@rtibbles rtibbles merged commit 08f375d into develop Nov 2, 2023
5 checks passed
@rtibbles rtibbles deleted the simplified_task_interface branch November 2, 2023 19:47
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.

3 participants