Enable key set ref counter and cleaner

[tucek]

This PR is based on PR #4439 branch and therefore has to be merged afterwards.

closes #4348 #3875 #4038 #3754

Basics

• [x] Short descriptions of your changes are in the release notes (added as entry in doc/news/_preparation_next_release.md which contains _(my name)_) Please always add something to the release notes.
• [x] Details of what you changed are in commit messages (first line should have module: short statement syntax)
• [x] References to issues, e.g. close #X, are in the commit messages.
• [x] The buildservers are happy. If not, fix in this order:
  • [x] add a line in doc/news/_preparation_next_release.md
  • [x] reformat the code with scripts/dev/reformat-all
  • [x] make all unit tests pass
  • [x] fix all memleaks
• [x] The PR is rebased with current master.

Checklist

• [ ] I added unit tests for my code
• [x] I fully described what my PR does in the documentation (not in the PR description)
• [x] I fixed all affected documentation
• [x] I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
• [ ] I updated all meta data (e.g. README.md of plugins and METADATA.ini)
• [ ] I mentioned every code not directly written by me in reuse syntax

Review

• [ ] Documentation is introductory, concise, good to read and describes everything what the PR does
• [ ] Examples are well chosen and understandable
• [ ] Code is conforming to our Coding Guidelines
• [ ] APIs are conforming to our Design Guidelines
• [ ] Code is consistent to our Design Decisions

Labels

• [ ] Add the "work in progress" label if you do not want the PR to be reviewed yet.
• [x] Add the "ready to merge" label if the basics are fulfilled and no further pushes are planned by you.

[kodebach]

LGTM in principle, but a few tests to validate everything would be nice. However, I'm not entirely sure how to best test this. It probably needs a mix of Java and C code in tests.

Also I think we can just close #4439 and merge this directly, since #4439 alone doesn't bring any real value AFAICT.

[tucek]

LGTM in principle, but a few tests to validate everything would be nice. However, I'm not entirely sure how to best test this. It probably needs a mix of Java and C code in tests.

Also I think we can just close #4439 and merge this directly, since #4439 alone doesn't bring any real value AFAICT.

Of cours we can close #4439, I just wanted to separate the clean-up from the more invasive change.

[tucek]

As discussed i will add a test for construct KeySet, getPointer, ksDel, ksDecRef, ksDel should succeed.

[markus2330]

Thank you! And please also do some manual testing with plugins, so that we know that in #4440 everything was working.

[tucek]

This PR is rdy to be merged if build errors are not related to my changes. see also https://github.com/ElektraInitiative/libelektra/pull/4439#issuecomment-1233570354

[markus2330]

jenkins build libelektra please

[markus2330]

jenkins build libelektra please

[markus2330]

Unfortunately, there is a merge conflict. Can you please rebase this PR?

[tucek]

rebased and to current elektra master and resolved conflicts

[tucek]

again, build is failing due to (another) unrelated error 🙄

set -- -DCMAKE_C_COMPILER=clang12 -DCMAKE_CXX_COMPILER=clang++12 -DBINDINGS='ALL;-io_glib' -DINSTALL_SYSTEM_FILES=ON -DCMAKE_SKIP_INSTALL_RPATH=ON -DCOMMON_FLAGS=-Werror -DC_STD=-std=c11 -DENABLE_ASAN="${ENABLE_ASAN:-OFF}" -DPLUGINS='ALL' -DTARGET_PLUGIN_FOLDER=''

printf '—— CMake Config ——\n'
—— CMake Config ——
for option; do printf '%s\n' "$option"; done
-DCMAKE_C_COMPILER=clang12
-DCMAKE_CXX_COMPILER=clang++12
-DBINDINGS=ALL;-io_glib
-DINSTALL_SYSTEM_FILES=ON
-DCMAKE_SKIP_INSTALL_RPATH=ON
-DCOMMON_FLAGS=-Werror
-DC_STD=-std=c11
-DENABLE_ASAN=OFF
-DPLUGINS=ALL
-DTARGET_PLUGIN_FOLDER=

cmake "$@" -G Ninja -B "$BUILD_DIR" .
CMake Error at CMakeLists.txt:15 (project):
  Running

   '/usr/local/bin/ninja' '--version'

  failed with:

   ld-elf.so.1: /lib/libc.so.7: version FBSD_1.7 required by /usr/local/bin/ninja not found

[mpranj]

Sorry, please rebase again. The error should be fixed on master with e3c31c738d3666878635741f2c97e979503f2f23.

[tucek]

@mpranj thank you for the fix, rebase done. let's see what ci says ;)

[markus2330]

jenkins build libelektra please

[markus2330]

There was a "No space left on device" error.

[tucek]

I've rebased the branch to current master.

[tucek]

That's more interesting this time:

160/271 Test  #34: testjna_gradle ................................***Failed    7.34 sec
double free or corruption (out)

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':libelektra:test'.

> Process 'Gradle Test Executor 1' finished with non-zero exit value 134
  This problem might be caused by incorrect test process configuration.
  Please refer to the test execution section in the User Manual at https://docs.gradle.org/7.4/userguide/java_testing.html#sec:test_execution
* Try:
> Run with --stacktrace option to get the stack trace.
> Run with --info or --debug option to get more log output.
> Run with --scan to get full insights.
* Get more help at https://help.gradle.org/

BUILD FAILED in 6s

[markus2330]

Can you please run with valgrind and/or the recommended options for gradle?

[kodebach]

Can you please run with valgrind and/or the recommended options for gradle?

If you have a concrete suggestion how to, please share. I have now tried multiple ways of running valgrind (with gradle, directly call java). I also tried using jemalloc, compiling with ASAN and also the memleak tool from bcc-tools. In all cases, I either got no memory leaks at all, only useless reports about parts of the JVM itself or the approach didn't work at all.

Interestingly, I have also not managed to reproduce the "double free" crash.

[markus2330]

jenkins build libelektra please

[tucek]

is based on PR #4898 which may be merged before separately

[tucek]

jenkins build libelektra please

[markus2330]

Wow, it succeeds! What has changed?

[tucek]

Wow, it succeeds! What has changed?

I've just updated all dependencies and fixed some minor bugs. The only thing i can imagine having any significant impact on the free problem is not accessing native resources after creation of a KDBException which lead to secondary exceptions when trying to evaluate the primary one. This was definitely a bug. But other than that i would also say just Wow ;)