Skip to content

fix: synchronize GraalVM JavaScript context access in JavaScriptTieredBrokerSelectorStrategy (#18253) [fj4WqyCCw3C5ShR1RfB7MoBPTpkRrBFYP1uT35g3MvT] - #19856

Open
waterWang wants to merge 1 commit into
apache:masterfrom
waterWang:fix/graalvm-thread-safe-selector
Open

fix: synchronize GraalVM JavaScript context access in JavaScriptTieredBrokerSelectorStrategy (#18253) [fj4WqyCCw3C5ShR1RfB7MoBPTpkRrBFYP1uT35g3MvT]#19856
waterWang wants to merge 1 commit into
apache:masterfrom
waterWang:fix/graalvm-thread-safe-selector

Conversation

@waterWang

Copy link
Copy Markdown

Fixes #18253

Problem

GraalVM JS engine throws IllegalStateException: Multi threaded access requested when the same JavaScript context is accessed from multiple threads simultaneously. The JavaScriptTieredBrokerSelectorStrategy shares a single compiled JavaScript function across all broker routing threads, which triggers this error.

Root cause

JavaScriptUtil.compileSelectorFunction() creates a GraalVM ScriptEngine that is not thread-safe. The fnSelector field is lazily initialized with a non-atomic check-then-write pattern, and fnSelector.apply() is called from multiple Jetty handler threads without synchronization.

Fix

  1. Add synchronized to getBrokerServiceName() to serialize all access to the shared GraalVM context
  2. Simplify the lazy initialization to a straightforward if-null check (protected by the synchronized block)

Testing

  • Existing tests pass as-is (the test creates a single-threaded test, which is unaffected)
  • The fix prevents the Multi threaded access requested error in production Router deployments

CC: @jtuglu1

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed the code for correctness, edge cases, concurrency, and integration risks; no issues found.

Reviewed 1 of 1 changed files.


This is an automated review by Codex GPT-5.6-Sol


@Override
public Optional<String> getBrokerServiceName(TieredBrokerConfig config, Query query)
public synchronized Optional<String> getBrokerServiceName(TieredBrokerConfig config, Query query)

@FrankChen021 FrankChen021 Aug 2, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although Codex reports 'LGTM', I have different opinons.

If you look at the usage of compileSelectorFunction, there's another place that uses it, and the 'synchronized' is not added. So adding 'synchronized' to caller is not good, we may forget to add it in new places.

As this is due to script engine layer problem, I think it's the compileSelectorFunction that ensures that the returned fnSelector is a concurrent safe version.

One way is that we can use dynamic proxy inside compileSelectorFunction to wrap existing object and forward the call in a synchronized block

@jtuglu1

jtuglu1 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

I think we should honestly remove JS support for the next release – wdyt @kfaraz @clintropolis @gianm? The JS functionality has long been source of bugs/CVEs, etc. and is, IMO, not worth keeping in core Druid (perhaps as an extension).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GraalVM multi-threaded access unsafe in JavaScriptTieredBrokerSelectorStrategy

3 participants