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.

Sign up for GitHub

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

Jump to bottom

Fix Gdx.files being garbage collected in MultiplayerTurnCheckWorker #6817

Merged
merged 3 commits into from
May 18, 2022
Merged

Fix Gdx.files being garbage collected in MultiplayerTurnCheckWorker #6817

merged 3 commits into from
May 18, 2022

Conversation

Azzurite
Copy link
Collaborator

@Azzurite Azzurite commented May 15, 2022 •
edited
Loading

So the Worker in Android can be freely stopped & recreated by the WorkManager. It is supposed to be a standalone "application" that can handle re-initialization.

We do this slightly unconventional thing of making a new OneTimeWorkRequestBuilder recursively (every time it runs, a new work request is made for the next execution) instead of using PeriodicWorkRequestBuilder. I assume that because of that, our first Worker instance we started has been kept alive for longer than it was supposed to, and thus no new instance was ever created with usual player behavior.

But apparently after a long time (multiple hours of the turn worker running on my phone), our Worker instance is stopped temporarily and then re-initialized by the WorkManager. So all the initialization that happened by AndroidLauncher (which is an AndroidApplication) that initialized the GDX variables is gone. This includes Gdx.files. Then, when the new Worker is created, a new GameSaver instance is also created. But this instance has so far been statically accessing Gdx.files, which is now gone.

So what the solution is here is to create & pass the correct [gdx].Files instance to the GameSaver on creation, instead of letting it use Gdx.files statically.

Exception 
java.lang.NullPointerException: Attempt to invoke interface method 'com.badlogic.gdx.files.FileHandle com.badlogic.gdx.Files.local(java.lang.String)' on a null object reference
 at com.unciv.logic.GameSaver.getSave(GameSaver.kt:36)
 at com.unciv.logic.GameSaver.saveGame(GameSaver.kt:91)
 at com.unciv.logic.GameSaver.saveGame$default(GameSaver.kt:90)
 at com.unciv.app.MultiplayerTurnCheckWorker$doWork$1.invokeSuspend(MultiplayerTurnCheckWorker.kt:268)
 at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
 at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
 at kotlinx.coroutines.EventLoopImplBase.processNextEvent(EventLoop.common.kt:279)
 at kotlinx.coroutines.BlockingCoroutine.joinBlocking(Builders.kt:85)
 at kotlinx.coroutines.BuildersKt__BuildersKt.runBlocking(Builders.kt:59)
 at kotlinx.coroutines.BuildersKt.runBlocking(Unknown Source:1)
 at kotlinx.coroutines.BuildersKt__BuildersKt.runBlocking$default(Builders.kt:38)
 at kotlinx.coroutines.BuildersKt.runBlocking$default(Unknown Source:1)
 at com.unciv.app.MultiplayerTurnCheckWorker.doWork(MultiplayerTurnCheckWorker.kt:238)
 at androidx.work.Worker$1.run(Worker.java:86)
 at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
 at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
 at java.lang.Thread.run(Thread.java:919)
Old description It seems like since the addition of `runBlocking` in `MultiplayerTurnCheckWorker`, the `AndroidApplication` that initially started the background worker gets garbage collected. Which is actually as it should be imo. But the `GameSaver` still needs to work, which is why we need to add a reference to `Gdx.files` to it so it doesn't get garbage collected.

@Azzurite Azzurite marked this pull request as draft May 15, 2022 13:14
@Azzurite Azzurite force-pushed the fix-turn-check-gc branch from 1388de4 to 968118a Compare May 15, 2022 13:21
@Azzurite Azzurite marked this pull request as ready for review May 15, 2022 13:22
@Azzurite Azzurite marked this pull request as draft May 15, 2022 14:18
@Azzurite
Copy link
Collaborator Author

This is a little annoying to test, because it only occurs after like ~1 hour of running in the background.

Apparently this was not enough. Probably got to save the GameSaver instance within the worker...

@SomeTroglodyte
Copy link
Collaborator

Apparently this was not enough

Makes me curious at to what the symptoms actually look like - or what you see/don't see to determine "enough"

@Azzurite
Copy link
Collaborator Author

I misunderstood how android Worker works. Will fix later.

@Azzurite Azzurite force-pushed the fix-turn-check-gc branch from 0e4d65e to 9b0bb1f Compare May 17, 2022 11:17
@Azzurite
Copy link
Collaborator Author

Azzurite commented May 17, 2022 •
edited
Loading

I have a fix in here now, but I want to refactor GameSaver from being a singleton to a simple class that we hold one instance to, so we don't have to do this GameSaver(); GameSaver.init(...) and can just do GameSaver(...) which ensures that init "is always called"

@Azzurite
Copy link
Collaborator Author

Azzurite commented May 17, 2022 •
edited
Loading

Actually there's no fucking reason to do that in this PR, please review :)

@Azzurite Azzurite marked this pull request as ready for review May 17, 2022 11:42
@xlenstra xlenstra mentioned this pull request May 17, 2022
Closed
@SomeTroglodyte
Copy link
Collaborator

no fucking reason

If you're looking for excuses for skin shenanigans on github, sorry, wrong place.

I ass-ume the conflicts are from #6847?

@Azzurite
Merge remote-tracking branch 'origin/master' into fix-turn-check-gc
4f124b6 
# Conflicts:
#	android/src/com/unciv/app/MultiplayerTurnCheckWorker.kt
#	core/src/com/unciv/logic/GameSaver.kt
@Azzurite
Copy link
Collaborator Author

I think conflicts came from #6822, but auto-resolve fixed all of them

@yairm210 yairm210 merged commit 5353f33 into yairm210:master May 18, 2022
@Azzurite Azzurite deleted the fix-turn-check-gc branch May 18, 2022 05:33
@Azzurite Azzurite mentioned this pull request May 22, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SomeTroglodyte SomeTroglodyte SomeTroglodyte left review comments

@GGGuenni GGGuenni GGGuenni approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Azzurite @SomeTroglodyte @GGGuenni @yairm210