Scripting Engine Debugger Support

[rjd15372]

This PR introduces the support for implementing remote debuggers in scripting engines modules.

The module API is extended with scripting engines callbacks and new functions that can be used by scripting engine modules to implement a remote debugger.

Most of the code that was used to implement the Lua debugger, was refactored and moved to the scripting_engine.c file, and only the code specific to the Lua engine, remained in the debug_lua.c file.

The SCRIPT DEBUG (YES|NO|SYNC) command was extend with an optional parameter that can be used to specify the engine name, where we want to enable the debugger. If no engine name is specified, the Lua engine is used to keep backwards compatibility.

In src/valkeymodule.h we see the module API changes. And in the helloscripting.c file we can see how to implement a simple debugger for the dummy HELLO scripting engine.

[codecov]

Codecov Report

❌ Patch coverage is 46.84685% with 354 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.37%. Comparing base (7e0b3bb) to head (28c9b2e).
⚠️ Report is 3 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/scripting_engine.c	48.35%	236 Missing ⚠️
src/lua/debug_lua.c	43.07%	74 Missing ⚠️
src/module.c	20.83%	19 Missing ⚠️
src/eval.c	50.00%	9 Missing ⚠️
src/valkey-cli.c	0.00%	9 Missing ⚠️
src/lua/engine_lua.c	76.00%	6 Missing ⚠️
src/server.c	66.66%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1701      +/-   ##
============================================
- Coverage     72.58%   72.37%   -0.22%     
============================================
  Files           128      128              
  Lines         71326    71626     +300     
============================================
+ Hits          51772    51839      +67     
- Misses        19554    19787     +233     

Files with missing lines	Coverage Δ
src/commands.def	100.00% <ø> (ø)
src/server.c	88.43% <66.66%> (ø)
src/lua/engine_lua.c	86.48% <76.00%> (-1.64%)	⬇️
src/eval.c	87.91% <50.00%> (-1.33%)	⬇️
src/valkey-cli.c	54.53% <0.00%> (-1.69%)	⬇️
src/module.c	9.83% <20.83%> (+0.04%)	⬆️
src/lua/debug_lua.c	37.11% <43.07%> (-2.17%)	⬇️
src/scripting_engine.c	53.60% <48.35%> (-19.83%)	⬇️

... and 11 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.

[madolson]

Postponing until 9.0.

[zuiderkwast]

I had this pending comment since forever. Posting it now.

We talked about this in a core team meeting. We said that nobody uses the Lua debugger and that we could basically deprecate it. No meed to make this large change to extend the debugger support. The main reason to do it is to avoid breaking it if we move Lua to a module?

[rjd15372]

We talked about this in a core team meeting. We said that nobody uses the Lua debugger and that we could basically deprecate it. No meed to make this large change to extend the debugger support. The main reason to do it is to avoid breaking it if we move Lua to a module?

Correct. This is required if we want to keep the debugger feature for Lua when moving Lua to a module.
I'm also fine to deprecate the debugger feature, but if that is the case we should deprecate it in 9.0 and raise somekind of warning to the user when he tries to use the debugger.

[zuiderkwast]

I like to see Lua moved into a module. To keep backward compat behaviour here, we need to make it load by default and add some new config to disable loading the default modules. But that's a future PR.

Regarding deeping or deleting the debugger, I think it's safer to keep the debugger, so I guess we should get this merged.

I'm fine with merging this without too detailed review, as long as it has been tested. Preferably we should test the ABI compatibility too, at least manually. Old Valkey should ignore new module's debugger callbacks. New Valkey should work (not crash on SCRIPT DEBUG, etc.) with module compiled for old valkeymodule.h.

[rjd15372]

@zuiderkwast I've updated the PR, and addressed your comments. Added new tests and fixed some bugs that appeared in the tests.

[rjd15372]

@zuiderkwast I've updated this PR with the current unstable branch. We should get this PR merged so I can open the PR that moves the lua engine into a module.

[zuiderkwast]

Yes, let's get this merged.

You need to commit the generated file commands.def.

[zuiderkwast]

I would like to get @madolson's eyes on the module API before we merge this.

[zuiderkwast]

Speed review, but it seems safe enough. I'm not worried about the debugger, but only about breaking the regular scripting with the refactoring. We have good test coverage for scripting though so I think it's safe.

[rjd15372]

@madolson do you want to review the module API additions?

[zuiderkwast]

I believe we can merge this without waiting for the others to review it, so we can unblock the other work. If there are any concerns regarding the module API, we have time to change it until 9.1.

It needs some rebase now and bump the scripting engine API version to 3?

[rjd15372]

@zuiderkwast yes, I've fixed the merge conflicts and updated the code accordingly. Run a few more tests and looks good. Will merge this now.