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

Possible fix for ANR during shutdown #6172

Merged
merged 9 commits into from Nov 26, 2021
Merged

Possible fix for ANR during shutdown #6172

merged 9 commits into from Nov 26, 2021

Conversation

AGulev
Copy link
Contributor

@AGulev AGulev commented Nov 9, 2021

No description provided.

@AGulev AGulev requested a review from JCash November 9, 2021 08:49
if (_glfwWinAndroid.app->destroyRequested) {
androidDestroyWindow();
return;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this condition (466-469) is a new piece of code, everything else indentations.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming that it's ok to exit the app right here then. We don't need to wait for threads? E.g. sound.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure.
As I understand from code

_glfwWin.opened = 0;
is a soft exit.

Our loop continue work but see this flag and finish all the threads.

But I'm not sure if it's fine or not.
In Android examples they not just exit Looper but exit android_main() to continue android_app_entry() function in app_glue and call android_app_destroy()

I can do the same, but that means I should change args.m_Finished = 1; to exit our loop in engine_main(). that means this logic should be in two places:

  1. in glfwAndroidPollEvents to exit looper
  2. in engine_main -> while (!args.m_Finished) to change args.m_Finished = 1; and exit loop there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added exit from the loopengine_main() loop as well. But still not sure if we should force exit app

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess, with the latest update, you are sure?

@AGulev AGulev self-assigned this Nov 9, 2021
JCash
JCash previously approved these changes Nov 9, 2021
@@ -106,7 +106,7 @@ namespace dmThread
* Detach thread. When a detached thread terminates, its resources are
* automatically released back to the system without the need for another
* thread to join with the terminated thread.
* @name dmThread::Join
* @name dmThread::Detach
Copy link
Contributor Author

Choose a reason for hiding this comment

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

typo in docs

engine/engine/src/engine_main.cpp Show resolved Hide resolved
@AGulev AGulev requested a review from JCash November 10, 2021 16:19
@@ -117,6 +117,9 @@ int engine_main(int argc, char *argv[])
{
glfwAndroidPollEvents();
dmTime::Sleep(0);
if (g_AndroidApp->destroyRequested) {
return 0;
Copy link
Contributor Author

@AGulev AGulev Nov 15, 2021

Choose a reason for hiding this comment

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

App requested exit. So, let's exit. I don't wait when thread work finished because app is in background already (can't close app from withing the app itself, only using OS functions)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a good code comment, so that others in the future can reason about the return statement.

JCash
JCash previously approved these changes Nov 15, 2021
@@ -117,6 +117,9 @@ int engine_main(int argc, char *argv[])
{
glfwAndroidPollEvents();
dmTime::Sleep(0);
if (g_AndroidApp->destroyRequested) {
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a good code comment, so that others in the future can reason about the return statement.

if (_glfwWinAndroid.app->destroyRequested) {
androidDestroyWindow();
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess, with the latest update, you are sure?

@AGulev
Copy link
Contributor Author

AGulev commented Nov 25, 2021

@JCash I added comments ad ready to merge it

@AGulev AGulev merged commit cbfdee6 into dev Nov 26, 2021
@AGulev AGulev deleted the fix/anr-2 branch November 26, 2021 14:07
@britzl britzl changed the title Possible fix for ANR #2 Possible fix for ANR during shutdown Nov 30, 2021
britzl added a commit that referenced this pull request Dec 10, 2021
…#6230)

* Possible ANR fix (#6160)

* timeout is in milliseconds

* exit ALooper_pollAll loop if app destroy requested

* fix wrong structure name

* Revert "fix wrong structure name"

This reverts commit efc2686.

* Revert "exit ALooper_pollAll loop if app destroy requested"

This reverts commit 653b17f.

* Restrict PRs to Defold org

* Update main-ci.yml

* Changed encoding (#6163)

* Break out of loop when printing stack trace (#6156)

* Check for resource key in both local and remote cache (#6148)

* Check for key in both local and remote cache

* Forgot to throw exception

* Issue-6151 Template override with layouts bug (#6166)

* Apply Default layout before applying another layout overridings

* Test

* fix naming

* Tests polishment after review

* New onCreate hook for Android in dmSDK (and breaking change for RegisterAndroidOnActivityResultListener) (#6171)

* onCreate hook for android

* Review fixes

* fix types

* Added OP Games logo

skip-ci

* Add support for DualSense Wireless Controller (mac)

* Ignore some project options when calculating cache key (#6159)

* Ignore some project options when calculating cache key

* Associate bob options with how the option impacts the resource cache

* Changed from ignoreOption to includeOption

* Update Bob.java

* Fixed resource cache key tests

* Update ResourceCacheKeyTest.java

* Code cleanup

* Bumped version to 1.2.190

* Make sure to create vertex declarations first time a custom buffer is set (#6178)

* Added the generated texture set protobuf output to the AtlasNode (#6182)

* Added the generated texture set protobuf output to the AtlasNode

* review fix

* Added release notes for console platforms (#6187)

* Added support for ABGR in texc, in order to skip a java data copy loop (#6192)

* Added support for ABGR in texc, in order to skip a java data copy loop

Saved ~20-25% in the case of the spineboy atlas

* Update README_SETUP_LINUX.md

* Issue 6199: Fixed issue of getting/setting render constants on label (#6200)

* Update README_SETUP_LINUX.md

* Avoid using memset on hashtable/array members (#6197)

* Issue 6189: Added dmSpinlock to dmSDK (#6196)

* Issue 6189: Added dmSpinlock to dmSDK

* typo

* Updated NE plugin build rules to work for macOS/Win32/Linux (#6202)

* Updated to jna-5.10.0 for improved error messages

* Added jna+jna-platform jar files

* moved files

* Updated linkCmdCXXSh to support start/end groups for different platforms

* Fix implicit cast of jobject to ANativeActivity (#6209)

* fix implicit cast of jobject to ANativeActivity

* Revert "fix implicit cast of jobject to ANativeActivity"

This reverts commit e9d8c83.

* use Activity jobject instead of ANativeActivity

* revert `jobject thiz` changes

* Issue 6210: Fix when setting more than 4 render constants (#6211)

* Added documentation example for gui.get_font_resource() (#6212)

* Remove mobile dev app (#6208)

* Don't build the mobile dev app

* Update waf_dynamo.py

* Cleaned up script a bit more

* Removed apkc

* Removed go

* Change to apksigner

* Delete apkc

* Use correct string length and number format (#6204)

* Use correct string length

Also make sure to use same number format

* Compare tostring and concat result

* Improved calculation of max buffer length for vmath userdata

* Issue 3775 collision sticking - II (#6205)

* More flexible box2d velocityThreshold setting - ref #3775

Updatable from a respective game.project property

* Fixes for box2d velocity threshold overrides - ref #3775

- The feature works when editing game.projectc directly
- The role of the physics 'scale'property seems to affect when the threshold value
gets effective.

* 'velocityThreshold' project property for the editor and 'scale' compensation - ref #3775

* WIP: Testing: Added scale and velocityThreshold properties to test GamesysTest base class

* Testing: (WIP) VelocityThreshold test in gamesys - ref #3775

* Issue 3775: Wrapped testing code and final code review - ref #3775

* Initialize velocity threshold in physics context. Fixes test behavior - ref #3775

* Removed WebP remnants (#6213)

* Possible fix for ANR #2 (#6172)

* Possible fix for ANR #2

* typo fixes

* Exit the main loop

* change place where we call `androidDestroyWindow();`

* add missed include

* Revert "add missed include"

This reverts commit ea8f07c.

* Revert "change place where we call `androidDestroyWindow();`"

This reverts commit dc12019.

* immediate app exit when exit requested

* Added comments

* Issue-6089 Set go.property() for textures and fonts in `gui` component

* sent font (without deleting for now)

* Release resources

* Revert "sent font (without deleting for now)"

This reverts commit 2731c37.

* use key for hashtable parameters

* use dmArray for resorce pointers

* Revert "Revert "sent font (without deleting for now)""

This reverts commit af482bb.

* use `hash` instead of `char*` all the way as hashtable key

* documentation fix

* Review fixes

* review fixes

* go.get() fonts

* fix error reporting

* fix names

* better error handling

* better error handling

* get textures from gui component

* Add go.set() for textures in GUI component

* Documentation

* initial test

* cleanup tests

* fix uniform test

* fix test done counter

* fix test_done counter

* fix typo

* more tests for fonts

* go.set/go.get "textures" test

* remove unused array size

* fix operators order

* make sure property_options.m_HasKey == 0

* make sure property_options.m_HasKey == 0 for get function as well

* review fixes

* review fixes

* review fixes

* review fixes

* README.md: HTTP => HTTPS (#6219)

* Add function `gui.set_alpha()` and `gui.get_alpha()` (#6218)

* Added missing documentation for indices/keys in go.get/set (#6220)

* Added missing documentation for indices/keys in go.get/set

* review fixes

* Issue-5791 `gui.animate` doesn't set value if duration = 0 and delay > 0 (#6216)

* take into account delay of animation in gui

* remove comment

* make negative values consistent with how `go.animate` works

* test animate itself

* Add callback testing

* review fixes

* Split spinlock types into multiple files for easier integration to console repos (#6221)

* Added VSCode + Calva readme (#6226)

* Compile fix

* Update to windows-latest machines (#6225)

* Updated to windows-latest for CI

Use local python2 on macOS

* Updated to new version of setup-python action

* Ensure editor builds can see changes from pre-build hooks

Co-authored-by: Alexey Gulev <me@agulev.com>
Co-authored-by: Björn Ritzl <bjorn.ritzl@gmail.com>
Co-authored-by: JCash <mwesterdahl76@gmail.com>
Co-authored-by: Orestis Tsakiridis <otsakiridis@freemail.gr>
Co-authored-by: Kai <Schweinepriester@users.noreply.github.com>
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.

None yet

2 participants