Skip to content

Add Static Module Support#3392

Draft
eifrah-aws wants to merge 1 commit intovalkey-io:unstablefrom
eifrah-aws:static-lua
Draft

Add Static Module Support#3392
eifrah-aws wants to merge 1 commit intovalkey-io:unstablefrom
eifrah-aws:static-lua

Conversation

@eifrah-aws
Copy link
Contributor

@eifrah-aws eifrah-aws commented Mar 23, 2026

Add a build option to compile the Lua scripting engine as a static module and wire the server to load it directly at startup when enabled. The module load path now resolves on-load and on-unload entry points from the main binary, and the module lifecycle keeps those callbacks so unload works without a shared library handle.

The Lua module build was updated to support both static and shared variants, with the static path exporting visible wrapper symbols and linking the server with the module archive. While touching the Lua code, a few internal symbols were renamed for consistency and the monotonic time helper was clarified.

Note that this PR addresses the LUA module, but it can be applied to other "core" modules (like: Bloom, Json, Search and others). With this change, it will be easier to ship Valkey bundle with modules.

Areas touched:

  • CMake
  • Makefile
  • Lua scripting module
  • Core module loading

Generated by CodeLite

Add a build option to compile the Lua scripting engine as a static module and
wire the server to load it directly at startup when enabled. The module load
path now resolves on-load and on-unload entry points from the main binary, and
the module lifecycle keeps those callbacks so unload works without a shared
library handle.

The Lua module build was updated to support both static and shared variants,
with the static path exporting visible wrapper symbols and linking the server
with the module archive. While touching the Lua code, a few internal symbols
were renamed for consistency and the monotonic time helper was clarified.

* CMake
* Makefile
* Lua scripting module
* Core module loading

**Generated by CodeLite**

Signed-off-by: Eran Ifrah <eifrah@amazon.com>
@codecov
Copy link

codecov bot commented Mar 23, 2026

Codecov Report

❌ Patch coverage is 12.98701% with 67 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.36%. Comparing base (07554d3) to head (26fe874).
⚠️ Report is 1 commits behind head on unstable.

Files with missing lines Patch % Lines
src/module.c 10.66% 67 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #3392      +/-   ##
============================================
- Coverage     74.51%   74.36%   -0.16%     
============================================
  Files           130      130              
  Lines         72748    72817      +69     
============================================
- Hits          54207    54147      -60     
- Misses        18541    18670     +129     
Files with missing lines Coverage Δ
src/config.c 77.72% <100.00%> (+0.01%) ⬆️
src/module.h 0.00% <ø> (ø)
src/server.c 89.49% <ø> (+0.08%) ⬆️
src/server.h 100.00% <100.00%> (ø)
src/module.c 26.09% <10.66%> (-0.29%) ⬇️

... and 25 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ranshid ranshid added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Mar 23, 2026
@ranshid
Copy link
Member

ranshid commented Mar 23, 2026

No deep dive yet.

I think we need to clarify requirements before proceeding with a deep dive.

The original change aimed to:

  1. Enable users to define and use new scripting engines (e.g., LuaJIT, JavaScript)
  2. Allow users who don't use script commands (EVAL, EVALSHA) to compile and run Valkey without Lua support, avoiding frequent patching for Lua library vulnerabilities

For the 9.1 release, we must decide:

  1. Legacy Lua load/unload support: Should we support dynamically loading/unloading Lua? The current implementation allows unloading Lua (though it remains in the binary) to replace it with a different Lua module. This could enable users to dynamically "patch" Lua, but the old module persists in the code. Does this provide meaningful value?

  2. Observability: The new Lua currently appears in the module list. This may impact installations that verify module list output. This decision depends on /1 if Lua cannot be dynamically loaded/unloaded, should we list it like other modules?

@ranshid ranshid self-requested a review March 23, 2026 13:18
Comment on lines 175 to 183
@@ -183,7 +183,7 @@ monotime getMonotonicUs(void) {
}
Copy link
Member

@dvkashapov dvkashapov Mar 23, 2026

Choose a reason for hiding this comment

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

Probably unrelated to this PR but should we somehow unify/sync getMonotonicUs() implementation in lua context with getMonotonicUs() implementation in server? Right now by default we build with hardware monotonic clocks that should not be compared with POSIX clock_gettime(CLOCK_MONOTONIC, &ts); @zuiderkwast @JimB123 WDYT?

@madolson madolson moved this to Todo in Valkey 9.1 Mar 23, 2026
@madolson
Copy link
Member

Core team discussion:

  1. Is this is an important thing to do in 9.1?
    1. We think it's important for 9.1.
  2. We are fine with having the same observability.
  3. We are fine with unloading and not being able to reload it.
  4. If it's statically linked it's loaded automatically. If it's not statically linked it's not loaded automatically.
  5. Statically linked will the default. Review the makefile variable so that it's similar with TLS and RDMA.

@valkey-io/core-team and @rjd15372 specifically in case you have some thoughts.

See #2858 (comment) for some additional discussion.

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

Labels

run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP)

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

4 participants