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

[Core] Fix Kernel Destructor #11511

Closed
wants to merge 1 commit into from
Closed

Conversation

roigcarlo
Copy link
Member

📝 Description
Kratos kernel now cleans the list of applications registered and components once destroyed, preventing pointers to deleted instances of Components and no longer available applications from existing after calling the kernel initialization and application registry without unloading the shared lib.

Related #11506

🆕 Changelog

  • Fixing Kernel destruction leaving registered components and applications

@roigcarlo roigcarlo requested a review from a team as a code owner August 28, 2023 09:52
Copy link
Member

@loumalouomega loumalouomega left a comment

Choose a reason for hiding this comment

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

Thanks

@roigcarlo
Copy link
Member Author

roigcarlo commented Aug 29, 2023

@loumalouomega I've been looking for the error of this and as I suspected is very hard to fix without revamping the test, so I am afraid this needs to be included in the GTests PR and cannot be split.

The error happens because some applications ( specifically the ones that have custom custom workflows in c++ like GeoMechanics) have custom kernels.

So:
1 - Python loads kratos (from the testing script)
2 - Python loads all apps (from the testing script)
3 - Python tests are run
4 - A call is made so all cpp tests are run (because are registered in the kernel regardless of the app so they run mixed)
5 - When a GeoMechanics tests or something using a custom instance of the kernel is called and ends, the kernel gets uninitialized
6 - As nobody re-initializes the kernel tests crash or segfault.

So in order to fix this I would have to disable all the cpp tests with cpp workflows or remade the script so instead of using the python instance that is created with python run_all_tests.py uses a subprocess that reloads the kernel and runs the tests based on a list of suits ( the ones uses in the CPP macro)

I don't want to disable those tests in the meantime and I will not make the changes in the resting script because I will ditch them when making the GTests. So, if you are ok with the changes here, I will just close the PR and consider this part reviewer in the other PR.

As a note, the current behavior is the wrong one as all tests are assuming that the kernel is in a correct state after a tests is executed, which is a very bold assumption and could cause interference between all the tests,

@loumalouomega
Copy link
Member

@loumalouomega I've been looking for the error of this and as I suspected is very hard to fix without revamping the test, so I am afraid this needs to be included in the GTests PR and cannot be split.

The error happens because some applications ( specifically the ones that have custom custom workflows in c++ like GeoMechanics) have custom kernels.

So: 1 - Python loads kratos (from the testing script) 2 - Python loads all apps (from the testing script) 3 - Python tests are run 4 - A call is made so all cpp tests are run (because are registered in the kernel regardless of the app so they run mixed) 5 - When a GeoMechanics tests or something using a custom instance of the kernel is called and ends, the kernel gets uninitialized 6 - As nobody re-initializes the kernel tests crash or segfault.

So in order to fix this I would have to disable all the cpp tests with cpp workflows or remade the script so instead of using the python instance that is created with python run_all_tests.py uses a subprocess that reloads the kernel and runs the tests based on a list of suits ( the ones uses in the CPP macro)

I don't want to disable those tests in the meantime and I will not make the changes in the resting script because I will ditch them when making the GTests. So, if you are ok with the changes here, I will just close the PR and consider this part reviewer in the other PR.

As a note, the current behavior is the wrong one as all tests are assuming that the kernel is in a correct state after a tests is executed, which is a very bold assumption and could cause interference between all the tests,

Okay :')

@roigcarlo
Copy link
Member Author

Closing. See #11506

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.

2 participants