[jira] [Commented] (RAMPART-459) Performance hit in RampartMessageData when handling inbound data

["Andreas Martens (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/RAMPART-459?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18123424#comment-18123424 ] 

Andreas Martens commented on RAMPART-459:
-----------------------------------------

[~robertlazarski] ; is this on your radar already? Do you have an idea for a fix?

> 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: java-dev-unsubscribe@axis.apache.org
For additional commands, e-mail: java-dev-help@axis.apache.org