[
https://issues.apache.org/jira/browse/RAMPART-459?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18127057#comment-18127057
]
Andreas Martens commented on RAMPART-459:
-----------------------------------------
{{this didn't fix the problem entirely for me, this is because the opensaml
init is actually {_}failing{_}! Turns out we have a classpath problem so
opensaml is getting our xerces parser which doesn't implement
{{{}jdk.xml.maxElementDepth{}}}. So in order to get myself further (so i can
ship a fix to a customer) before dealing with the subsequent problems, I've
added this dirty, dirty hack:}}
{{}}
{{diff --git
i/modules/rampart-trust/src/main/java/org/apache/rahas/impl/util/OpenSAMLInitializer.java
w/modules/rampart-trust/src/main/java/org/apache/rahas/impl/util/OpenSAMLInitializer.java}}
{{index 2b30ed79..2021c363 100644}}
{{---
i/modules/rampart-trust/src/main/java/org/apache/rahas/impl/util/OpenSAMLInitializer.java}}
{{+++
w/modules/rampart-trust/src/main/java/org/apache/rahas/impl/util/OpenSAMLInitializer.java}}
{{@@ -47,23 +47,51 @@ public final class OpenSAMLInitializer { private
static volatile boolean initialized;+ /**}}
{{+ * The failure of the one initialisation attempt, if it failed. Failures
here are}}
{{+ * environmental and repeat identically (for example a standalone Xerces
on the}}
{{+ * classpath rejecting \{@code jdk.xml.maxElementDepth}), so retrying
per message}}
{{+ * only re-serialises every thread on the class monitor for the same
result.}}
{{+ */}}
{{+ private static volatile InitializationException failure;}}
{{+}}
{{ private OpenSAMLInitializer() {}}
{{ } /**}}
{{- * Initialises OpenSAML once. After the first successful call this is a
single}}
{{- * volatile read. A failed attempt is not recorded, so the next call
retries.}}
{{+ * Initialises OpenSAML once. After the first attempt, successful or
not, this is a}}
{{+ * volatile read and never takes the lock. If the attempt failed, every
later call}}
{{+ * rethrows that same failure without retrying.}}
{{ */}}
{{ public static void ensureInitialized() throws InitializationException {}}
{{ if (initialized) {}}
{{ return;}}
{{ }}}
{{+ InitializationException previous = failure;}}
{{+ if (previous != null) {}}
{{+ throw previous;}}
{{+ }}}
{{ synchronized (OpenSAMLInitializer.class) {}}
{{ if (initialized) {}}
{{ return;}}
{{ }}}
{{- OpenSAMLUtil.initSamlEngine();}}
{{- InitializationService.initialize();}}
{{+ if (failure != null) {}}
{{+ throw failure;}}
{{+ }}}
{{+ try {}}
{{+ OpenSAMLUtil.initSamlEngine();}}
{{+ InitializationService.initialize();}}
{{+ } catch (InitializationException e) {}}
{{+ failure = e;}}
{{+ throw e;}}
{{+ } catch (RuntimeException e) {}}
{{+ // Module initialisers can fail with unchecked exceptions,
which}}
{{+ // InitializationService does not wrap.}}
{{+ InitializationException wrapped =}}
{{+ new InitializationException("OpenSAML initialization
failed", e);}}
{{+ failure = wrapped;}}
{{+ throw wrapped;}}
{{+ }}}
{{ initialized = true;}}
{{ if (log.isDebugEnabled()) {}}
{{ log.debug("OpenSAML initialization complete");}}
Essentially I'm just rethrowing the exception rather than retrying init. I'll
remove this later. Since I don't actually _want_ OpenSAML it's fine for now (we
have external SAML handling that we implemented back in 2005-ish. Ultimately I
might try to convert us to OpenSAML, but that's a future problem.).
This has allowed the single-threaded performance to go from ~5msgs/s to ~50,
and I can scale it up to 800-900 msgs/s until the old VM I'm running on becomes
CPU-bound, so I don't _think_ there's another lock there holding us back.
> Performance hit in RampartMessageData when handling inbound data
> ----------------------------------------------------------------
>
> Key: RAMPART-459
> URL: https://issues.apache.org/jira/browse/RAMPART-459
> Project: Rampart
> Issue Type: Bug
> Components: rampart-core
> Affects Versions: 2.0.0
> Reporter: Andreas Martens
> Priority: Major
>
> I wanted to be able to submit a PR with a fix, but the
> {{PERFORMANCE vs CORRECTNESS:}}
> comment at
> [https://github.com/apache/axis-axis2-java-rampart/blob/29a48637306f93ea1474ce4e428ac94ac4a3beb9/modules/rampart-core/src/main/java/org/apache/rampart/RampartMessageData.java#L239-L268]
> scared me.
> What our problem is (I think introduced by RAMPART-454):
> If we're processing lots of inbound WS-Security messages, we get significant
> lock contention in e.g:
> {{"Thread-77" prio=5 Id=118 BLOCKED on java.lang.Class@1330ff3c owned by
> "Thread-83" Id=124 }}{{{}at
> org.opensaml.core.config.InitializationService.initialize(InitializationService.java:47){}}}{{{}-
> locked java.lang.Class@1330ff3c{}}}{{{}at
> org.apache.rampart.RampartMessageData.<init>(RampartMessageData.java:234){}}}{{{}at
> org.apache.rampart.RampartEngine.process(RampartEngine.java:100){}}}{{{}at
> org.apache.rampart.handler.RampartReceiver.invoke(RampartReceiver.java:125){}}}{{{}at
> org.apache.axis2.engine.Phase.invokeHandler(Phase.java:335){}}}{{{}at
> org.apache.axis2.engine.Phase.invoke(Phase.java:308){}}}{{{}at
> org.apache.axis2.engine.AxisEngine.invoke(AxisEngine.java:250){}}}{{{}at
> org.apache.axis2.engine.AxisEngine.receive(AxisEngine.java:156){}}}
>
> You can understand our annoyance at being stopped behind a SAML lock, when
> we're not using SAML!
> In the comment above the InitializationService.initialize() that's killing
> us, it says:
> {{Performance: all of these calls are idempotent guards}}
> but that appears to be incorrect for the SAML initialization (there's a TODO
> in there...).
> Explanation from my LLM which might make more sense than my rambling:
> >>>>>
> *Summary:* {{InitializationService.initialize()}} called unconditionally
> per-message causes thread contention under load
> *Description:*
> {{RampartMessageData(MessageContext, boolean)}} unconditionally calls
> {{org.opensaml.core.config.InitializationService.initialize()}} on every
> message, regardless of whether the active security policy involves SAML at
> all.
> {{InitializationService.initialize()}} is declared {{{}public static
> synchronized{}}}, acquiring a class-level monitor for its full duration.
> Critically, it contains *no idempotency guard* — on every invocation it
> constructs a fresh {{ServiceLoader<Initializer>}} and re-runs every
> registered module initialiser from scratch. (The OpenSAML authors acknowledge
> this themselves with a TODO in {{{}getServiceLoader(){}}}: {_}"ideally would
> store off loader and reuse on subsequent calls, avoiding re-initing
> problems."{_})
> Under concurrent load this produces the contention pattern visible in thread
> dumps: all message-processing threads serialise on
> {{{}java.lang.Class@<InitializationService>{}}}, with the holding thread
> running the full ServiceLoader scan and every other thread blocked waiting
> for it.
> The comment in the code acknowledges the per-message placement as a
> trade-off, but states _"all of these calls are idempotent guards"_ — this is
> incorrect for {{{}InitializationService.initialize(){}}}. The other two calls
> in the same block ({{{}WSSConfig.init(){}}} and
> {{{}OpenSAMLUtil.initSamlEngine(){}}}) *are* properly guarded with boolean
> flags and are effectively no-ops after the first call.
> {{InitializationService.initialize()}} is not.
> *Impact:* Affects all users regardless of their security policy. Non-SAML
> deployments pay the full cost of OpenSAML initialisation on every message
> with no benefit whatsoever.
> *Root cause:* The {{InitializationService.initialize()}} call should be
> removed from the per-message constructor path.
> {{OpenSAMLUtil.initSamlEngine()}} (already called immediately after)
> internally calls {{OpenSAMLBootstrap.bootstrap()}} and populates
> {{unmarshallerFactory}} — the original correctness concern that motivated
> this placement. Once {{samlEngineInitialized}} is {{{}true{}}},
> {{initSamlEngine()}} is a no-op and the unmarshallerFactory ordering issue
> cannot recur. The {{InitializationService.initialize()}} call is therefore
> redundant as well as harmful.
> The broader fix — moving all one-time initialisation to a module lifecycle
> hook rather than the per-message constructor — is already noted as the
> intended solution in the code comments.
> <<<<<
> I'll have a poke at the code to see whether I can come up with a fix, raising
> this issue for discussion...
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
