A way to disable the optimization for filling the stack traces

[neboskreb]

This PR addresses issue #3639

This PR allows to disable the optimization for filling the stack traces on platforms where it is not available (e.g. on Android).

The developer of the port to such platform shall call PrivateSecurityManagerStackTraceUtil.disable() before any loggins is printed, e.g. during the configuration phase.

For this reason, the visibility of PrivateSecurityManagerStackTraceUtil was increased to public.

The capability of instantiated PrivateSecurityManager is checked, and a faulty candidate (e.g. on Android) is rejected, thus rendering the optimization not enabled.

[github-actions]

Job	Requested goals	Build Tool Version	Build Outcome	Build Scan®
build-macos-latest	clean install	3.9.8	✅
build-ubuntu-latest	clean install	3.9.8	✅
build-windows-latest	clean install	3.9.8	✅

Generated by gradle/develocity-actions

[ppkarwasz]

@neboskreb,

Thank you for your contribution!

I think that PrivateSecurityManagerStackTraceUtil should always be disabled on Android. Similarly to #3071 I would simply disable the class if the java.vendor system property contains Android.

[neboskreb]

@ppkarwasz , as you mentioned in the other ticket, this PR seems to be on a critical path.

But I'm not quite sure I understand what is the blocker here, sorry. What can I do to unblock the progress?

[ppkarwasz]

But I'm not quite sure I understand what is the blocker here, sorry. What can I do to unblock the progress?

Currently you are proposing a new PrivateSecurityManagerStackTraceUtil#disable() method that can be used by other libraries to disable the security manager.

My point is that PrivateSecurityManagerStackTraceUtil will never work on Android, so we might as well always disable the class on that platform. More concretely the static initialization code:

logging-log4j2/log4j-api/src/main/java/org/apache/logging/log4j/util/PrivateSecurityManagerStackTraceUtil.java

Lines 31 to 44 in 7acbc48

	static {
	PrivateSecurityManager psm;
	try {
	final SecurityManager sm = System.getSecurityManager();
	if (sm != null) {
	sm.checkPermission(new RuntimePermission("createSecurityManager"));
	}
	psm = new PrivateSecurityManager();
	} catch (final SecurityException ignored) {
	psm = null;
	}
	SECURITY_MANAGER = psm;
	}

should:

• check if the Java property java.vendor contains the word Android.
• if we are running on Android set SECURITY_MANAGER to null.
• otherwise proceed as before.

[neboskreb]

Ah, I see your point. Let me quickly draft the solution.

[neboskreb]

Okay, so checking the runtime platform appears ugly, because it requires copying the code from SystemUtils (which is available in core but not here in api) or referring to core as dependency - neither being a great idea...

Therefore I decided, instead, to go with checking the capability of the instantiated PrivateSecurityManager. This looks more robust, because it's not the platform (we wouldn't be excited to add check isOsXxxx for every possible beast in the zoo) but the capability we're interested in.

I have completely rewritten the solution, and replaced the PR description.
@ppkarwasz please check and say what you think?

[ppkarwasz]

Okay, so checking the runtime platform appears ugly, because it requires copying the code from SystemUtils (which is available in core but not here in api) or referring to core as dependency - neither being a great idea...

I think that this should be enough:

boolean isAndroid = System.getProperty("java.vendor", "").contains("Android");

If we already know the System.getSecurityManager() is null, we don't even have to use a try...catch, since the getProperty() method can not throw.

[neboskreb]

Apparently, we have different approaches. Feel free to close this PR if you don't plan to merge it. After all, it proposes one of many possible ways to fix #3639; you can always draft a better solution yourself, especially that you already have the bigger part of it.

[neboskreb]

Re-formatted the code to make Spotless happy. Hopefully, this turns the build green.

This is the most I can do, I'm sorry.

[ppkarwasz]

LGTM, thanks!

[ppkarwasz]

@neboskreb,

Thanks! In order to merge the PR:

• All the commits must be signed with a GPG recognized by Github. If you didn't do it before, could you add a GPG key to your Github account and then rebase this PR, so all commits get signed?
• Could you add a changelog file to src/changelog/.2.x.x? You can use one of the files already there as template.

[neboskreb]

• Could you add a changelog file to src/changelog/.2.x.x? You can use one of the files already there as template.

Done

• All the commits must be signed with a GPG recognized by Github. If you didn't do it before, could you add a GPG key to your Github account and then rebase this PR, so all commits get signed?

Hmmm, git log --show-signature 40d66ed7 shows that the commit is signed but I don't see the checkmark next to it here on Github... Is this normal? If not, what can I do to rectify?

[ppkarwasz]

I don't see the signatures, I even checked the raw data.
Try

git rebase -f -i -S 2.x
git push -f

[neboskreb]

This helped!
Thanks a lot!

[ppkarwasz]

@neboskreb,

Thanks a lot! Now the Android support is almost as good as the JVM support! 💯

IIRC there is still one location-related feature that breaks on Android: LogManager.getLogger() will fail, since the Java 8 version of StackLocator relies on the absent sun.reflect.Reflection class and Android does not support Multi-Release JARs, so it doesn't use the Java 11 version of StackLocator.

If you know of a smart way to fix that, let me know.