[
https://issues.apache.org/jira/browse/RAMPART-459?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18125573#comment-18125573
]
Andreas Martens commented on RAMPART-459:
-----------------------------------------
Oh, and I'd only got as far as creating test cases for rampart with:
1. multi-threading to show the locking issue
2. proving the affected variable isn't null
the second is void now, but the first one might still be of value? My only
concern is that it's timing based, so if it runs on a slow infrastructure
machine it might still fail.
> 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]
