[
https://issues.apache.org/jira/browse/RAMPART-459?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18125325#comment-18125325
]
Robert Lazarski commented on RAMPART-459:
-----------------------------------------
Thanks, your analysis was correct: InitializationService.initialize() has no
idempotency guard in
OpenSAML 5, so every message re-ran OpenSAML's initializers under a
class-level lock.
Fixed on master in 6d19d990. OpenSAML is now initialized once, via a new
OpenSAMLInitializer,
rather than per message; it also covers the same call in SAML2Utils. Simply
deleting the call
wasn't safe, as initSamlEngine() replaces OpenSAML's global configuration and
the per-message
call was what repopulated it.
These commits were tested against Axis2/Java 2.0.2-SNAPSHOT. The Axis2 2.0.2
release vote is
about to start on the dev list, with a Rampart release to follow. I help with
Rampart as part of
my Axis PMC role but don't run it in production myself, so a thread dump from
your load test
against a master build would be very useful before the Rampart release. Note
that Axis2 2.0.2 no
longer bundles Guava, so copy guava and failureaccess from the Rampart
distribution's lib/ as
well.
> 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]
