Andreas Martens created RAMPART-459:
---------------------------------------
Summary: 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
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]
