Python 脚本引擎 支持#1742
Closed
paulzzh wants to merge 13 commits into
Closed
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough引入基于 Jep 的 Python 脚本引擎（编译、执行、线程安全解释器），在 ScriptEngineManager 注册 ".py" 支持，新增 CompiledScript.close() 生命周期方法，更新 Web UI 支持脚本类型选择，移除若干 Aviator 测试脚本及测试依赖，调整 Main.getPlatform() 可见性。 Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Web UI
participant Manager as ScriptEngineManager
participant PyEngine as PyScriptEngine
participant PyCompiled as PyCompiledScript
participant Interpreter as PyThreadSafeInterpreter
participant Jep as Jep Interpreter
UI->>Manager: 请求编译脚本(.py)
Manager->>PyEngine: compileScript(file, content)
PyEngine->>PyEngine: 恶意代码扫描与头部元数据解析
PyEngine->>Interpreter: 提交编译/加载任务
Interpreter->>Jep: 在单线程执行器中运行/编译
Jep-->>Interpreter: 返回编译对象
PyEngine-->>Manager: 返回 PyCompiledScript
UI->>Manager: 请求执行脚本
Manager->>PyCompiled: execute(env)
PyCompiled->>Interpreter: 获取 Lock 并注入 env
Interpreter->>Jep: 执行编译对象
Jep-->>Interpreter: 返回 result
PyCompiled-->>Manager: 返回执行结果
UI->>PyCompiled: close()
PyCompiled->>Interpreter: shutdown()/释放资源
Interpreter->>Jep: 关闭解释器
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 分钟 Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求旨在通过引入Python脚本引擎支持来增强系统的自定义规则功能。现在，用户不仅可以使用AviatorScript，还可以利用Python 2的强大功能来编写更复杂、更灵活的规则。后端通过集成Jep库和实现一套新的脚本编译与安全执行机制来支持这一功能，而前端则提供了直观的界面，允许用户轻松选择和管理不同类型的脚本。这一改进显著提升了系统的可扩展性和用户自定义能力。 Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
coderabbitai
Bot
added
enhancement
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This PR introduces support for a Python script engine using the Jep library, enabling the creation and editing of both Aviator and Python scripts. While this adds powerful customization, it also introduces significant security risks. The primary concern is the lack of sandboxing in PySafeInterpreter, which allows for arbitrary code execution (RCE) with the application's privileges, along with vulnerabilities to Denial of Service (DoS) attacks due to a lack of script isolation. Critically, the build.gradle.kts file has removed all test dependencies, severely impacting project quality. There's also a high-severity resource leak in PySafeInterpreter due to an unclosed ExecutorService. Other issues include a misleading 'Python 2' label in the UI for Python 3 support and general code quality improvements detailed in specific comments. Implementing robust sandboxing, ensuring script isolation, and improving error handling are crucial before merging.
Comment on lines
+11
to
+116
| public class PySafeInterpreter implements Interpreter { | ||
| private static final ReentrantLock lock = new ReentrantLock(); | ||
| private static final Interpreter interpreter; | ||
| private static final ExecutorService executor = Executors.newSingleThreadExecutor((r) -> { | ||
| Thread t = new Thread(r, "py-interpreter-thread"); | ||
| t.setDaemon(false); | ||
| return t; | ||
| }); | ||
|
|
||
| static { | ||
| try { | ||
| interpreter = executor.submit(SharedInterpreter::new).get(); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| throw new RuntimeException("Interrupted while initializing Python interpreter", e); | ||
| } catch (ExecutionException e) { | ||
| throw new RuntimeException("Failed to initialize Python interpreter", e.getCause()); | ||
| } | ||
| } | ||
|
|
||
| public PySafeInterpreter() { | ||
| lock.lock(); | ||
| } | ||
|
|
||
| private <T> T submitToInterpreter(Callable<T> task) throws JepException { | ||
| Future<T> f = executor.submit(task); | ||
| try { | ||
| return f.get(); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| throw new RuntimeException("Interrupted while waiting for Python task", e); | ||
| } catch (ExecutionException e) { | ||
| Throwable c = e.getCause(); | ||
| switch (c) { | ||
| case JepException jepException -> throw jepException; | ||
| case RuntimeException runtimeException -> throw runtimeException; | ||
| case Error error -> throw error; | ||
| case null, default -> throw new RuntimeException(c); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public Object invoke(String name, Object... args) throws JepException { | ||
| return submitToInterpreter(() -> interpreter.invoke(name, args)); | ||
| } | ||
|
|
||
| @Override | ||
| public Object invoke(String name, Map<String, Object> kwargs) throws JepException { | ||
| return submitToInterpreter(() -> interpreter.invoke(name, kwargs)); | ||
| } | ||
|
|
||
| @Override | ||
| public Object invoke(String name, Object[] args, Map<String, Object> kwargs) throws JepException { | ||
| return submitToInterpreter(() -> interpreter.invoke(name, args, kwargs)); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean eval(String str) throws JepException { | ||
| return submitToInterpreter(() -> interpreter.eval(str)); | ||
| } | ||
|
|
||
| @Override | ||
| public void exec(String str) throws JepException { | ||
| submitToInterpreter(() -> { | ||
| interpreter.exec(str); | ||
| return null; | ||
| }); | ||
| } | ||
|
|
||
| @Override | ||
| public void runScript(String script) throws JepException { | ||
| submitToInterpreter(() -> { | ||
| interpreter.runScript(script); | ||
| return null; | ||
| }); | ||
| } | ||
|
|
||
| @Override | ||
| public Object getValue(String str) throws JepException { | ||
| return submitToInterpreter(() -> interpreter.getValue(str)); | ||
| } | ||
|
|
||
| @Override | ||
| public <T> T getValue(String str, Class<T> clazz) throws JepException { | ||
| return submitToInterpreter(() -> interpreter.getValue(str, clazz)); | ||
| } | ||
|
|
||
| @Override | ||
| public void set(String name, Object v) throws JepException { | ||
| submitToInterpreter(() -> { | ||
| interpreter.set(name, v); | ||
| return null; | ||
| }); | ||
| } | ||
|
|
||
| @Override | ||
| public Interpreter attach(boolean shareGlobals) { | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public void close() { | ||
| lock.unlock(); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The PySafeInterpreter class, despite its name, lacks sandboxing and restrictions, enabling arbitrary Python code execution (RCE) with application privileges. This poses a critical security risk, as it allows access to system commands via os or subprocess and manipulation of Java classes via Jep's integration. Furthermore, the static executor (an ExecutorService) is not properly shut down, causing a resource leak and potentially preventing application termination. It is crucial to implement a robust sandbox for the Python interpreter (e.g., using a custom ClassEnquirer and restricting the Python environment) and to add a static shutdown() method for the executor to be called in the application's shutdown hook.
Member
|
前端应该已经有了，改一下就行 |
Member
|
另外要不要考虑一下lua |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webui/src/views/custom-script/components/detailDrawer.vue (1)
94-108:⚠️ Potential issue | 🟡 Minor新建脚本时没有重置
fileType。如果上一次打开的是
.py脚本，再点“新增脚本”，这里会继承上一次的类型。默认后缀和编辑器语言都会跟着跑偏。🛠️ 建议修改
viewDetail: (id: string | undefined, readonly: boolean = false) => { if (!id) { isNew.value = true form.name = '' + form.fileType = 'av' content.value = '' viewOnly.value = false } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webui/src/views/custom-script/components/detailDrawer.vue` around lines 94 - 108, The viewDetail handler fails to reset form.fileType when creating a new script, causing the new form to inherit the previous file type; update viewDetail (the branch where id is falsy) to explicitly set form.fileType to the default (e.g., 'av' or whichever app default is used) and ensure any editor-language state tied to fileType (e.g., content/editor language bindings) is also reset so the newly opened "new script" uses the correct default suffix and editor mode; keep the existing resets for isNew, form.name, content.value, and viewOnly and add the fileType reset in that same branch.
🧹 Nitpick comments (1)
src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PySafeInterpreter.java (1)
31-33: 构造函数加锁模式说明这种设计模式将
PySafeInterpreter实例作为锁的作用域，配合close()释放锁，在 try-with-resources 中使用是安全的。但需要注意：如果构造后没有调用close()（例如异常路径），锁将不会被释放，导致死锁。建议在文档中明确说明必须在 try-with-resources 或 finally 块中使用此类。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PySafeInterpreter.java` around lines 31 - 33, The constructor for PySafeInterpreter acquires a lock (lock.lock()) but if close() isn't called (e.g., on exceptions) the lock is never released; update the class documentation and API to make safe usage explicit: add JavaDoc on the PySafeInterpreter class and its constructor stating callers must use try-with-resources or call close() in a finally block, and ensure the class implements AutoCloseable (and reference the existing close() method) so try-with-resources is possible; optionally mention behavior of lock acquisition in the doc so callers know the constructor blocks until the lock is obtained.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/ghostchu/peerbanhelper/Main.java`:
- Around line 118-120: The getPlatform() getter lost its nullability annotation
causing callers to assume non-null; restore `@Nullable` on the getPlatform()
method (using the project's existing Nullable annotation type, e.g.,
javax.annotation.Nullable or org.jetbrains.annotations.Nullable) so callers and
Kotlin code keep correct null-safety—update the import if needed and ensure the
annotation is placed on the public static Platform getPlatform() method that
returns the platform field which loadPlatform() may set to null on non-Windows.
In
`@src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyCompiledScript.kt`:
- Around line 25-35: The code in PyCompiledScript.execute is evaluating an
object compiled with mode 'exec' using eval("eval(compiled_code)"), which won't
execute 'exec'-mode code; change the execution call to use exec instead: in the
PyCompiledScript.execute method (and where compiledCode is set into the
interpreter via interpreter.set("compiled_code", compiledCode)), replace the
eval(...) invocation with an exec(...) invocation (e.g., call the interpreter's
exec API or interpreter.eval("exec(compiled_code)") if the interpreter exposes
exec via eval), so the compiled 'exec' code actually runs and then return
interpreter.getValue("result").
In
`@src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PySafeInterpreter.java`:
- Around line 107-110: 在 PySafeInterpreter 类中，attach(boolean shareGlobals) 目前返回
null，可能导致调用方空指针异常；请将该方法改为抛出
UnsupportedOperationException（或其他合适的运行时异常）以显式表明不支持该操作，引用方法名 attach(boolean
shareGlobals) 和 接口 Interpreter 来定位并替换当前的返回 null 实现，并在异常消息中说明该解释器不支持 attach
操作以便调用方能更清楚原因。
- Around line 11-18: The ExecutorService "executor" in PySafeInterpreter is
created with non-daemon threads and no shutdown, which will prevent JVM exit;
update the class to add a proper shutdown mechanism: either change the thread
factory to setDaemon(true) in the lambda that creates the
"py-interpreter-thread" or add a static shutdown hook that calls
executor.shutdownNow()/shutdown() and awaits termination (or expose an explicit
close/stop method on PySafeInterpreter that shuts down the executor), ensuring
the hook or method references the static "executor" field and handles
InterruptedException/await timeouts.
In
`@src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyScriptEngine.kt`:
- Around line 168-171: The catch block in PyScriptEngine.kt currently catches
Throwable type Error and thus misses JepException (which extends Exception);
update the exception handling in the compile/initialization routine to catch
Exception (or preferably the specific JepException) instead of Error so
compilation failures return null as intended; adjust the catch from "catch (e:
Error)" to "catch (e: Exception)" (or "catch (e: JepException)") and keep using
log.warn with fallbackName and the caught exception variable.
- Around line 152-166: The code wrongly calls PySafeInterpreter.getValue with a
Python expression string; change it to first execute a compile assignment (e.g.,
via PySafeInterpreter.exec to run "compiled = compile(script_content, filename,
'exec')") and then retrieve the compiled object with getValue("compiled");
update the block around PySafeInterpreter().use where compiledCode is set so it
execs the assignment and then calls getValue("compiled") to obtain the
compiledCode used to construct the PyCompiledScript.
In
`@src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/ScriptEngineManager.kt`:
- Around line 16-25: 构造函数和 init 块不应无条件注册 Python 引擎；请把 PyScriptEngine
的依赖改为可选（例如通过 ObjectProvider<PyScriptEngine> 或接受 nullable 参数）并仅在启动时经可用性探测后将 ".py"
注入到 ScriptEngineManager.engineMap；同时修复 PySafeInterpreter 的静态初始化（不要在静态块直接调用
SharedInterpreter::new() 抛出 RuntimeException）——改为在 PySafeInterpreter
提供一个可用性检查方法（try/except 捕获初始化错误并返回 false）并在 PyScriptEngine.compile()
使用该检查或在工厂/配置阶段捕获失败后禁用注册；可选地使用 Spring 的 `@ConditionalOnProperty` 来控制是否尝试创建/注册
PyScriptEngine，以保证系统能在缺少 Jep 原生库时降级运行而不崩溃。
In `@webui/src/views/custom-script/components/detailDrawer.vue`:
- Around line 31-35: 将模板中显示的 Python 标签从 "Python 2" 更新为反映实际支持的版本（例如改为 "Python" 或
"Python 3"），同时保留 radio 的 value="py"；并在创建新脚本的流程结束处（即处理新建脚本/提交的函数中）显式重置表单文件类型：将
form.fileType 赋回默认 'av'，以避免前一个脚本选择残留。确保修改涉及的符号包括 template 中的 a-radio 文本、radio
value="py" 和表单字段 form.fileType（以及提交/重置处理函数）。
---
Outside diff comments:
In `@webui/src/views/custom-script/components/detailDrawer.vue`:
- Around line 94-108: The viewDetail handler fails to reset form.fileType when
creating a new script, causing the new form to inherit the previous file type;
update viewDetail (the branch where id is falsy) to explicitly set form.fileType
to the default (e.g., 'av' or whichever app default is used) and ensure any
editor-language state tied to fileType (e.g., content/editor language bindings)
is also reset so the newly opened "new script" uses the correct default suffix
and editor mode; keep the existing resets for isNew, form.name, content.value,
and viewOnly and add the fileType reset in that same branch.
---
Nitpick comments:
In
`@src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PySafeInterpreter.java`:
- Around line 31-33: The constructor for PySafeInterpreter acquires a lock
(lock.lock()) but if close() isn't called (e.g., on exceptions) the lock is
never released; update the class documentation and API to make safe usage
explicit: add JavaDoc on the PySafeInterpreter class and its constructor stating
callers must use try-with-resources or call close() in a finally block, and
ensure the class implements AutoCloseable (and reference the existing close()
method) so try-with-resources is possible; optionally mention behavior of lock
acquisition in the doc so callers know the constructor blocks until the lock is
obtained.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1697e07a-e2fe-4d82-b998-c7578db25bf1
📒 Files selected for processing (22)
build.gradle.ktssrc/main/java/com/ghostchu/peerbanhelper/Main.javasrc/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyCompiledScript.ktsrc/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PySafeInterpreter.javasrc/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyScriptEngine.ktsrc/main/java/com/ghostchu/peerbanhelper/util/scriptengine/ScriptEngineManager.ktsrc/test/resources/testdata/scripts/aviator/client_name_check.avsrc/test/resources/testdata/scripts/aviator/complex_condition.avsrc/test/resources/testdata/scripts/aviator/invalid_syntax.avsrc/test/resources/testdata/scripts/aviator/ip_range_check.avsrc/test/resources/testdata/scripts/aviator/peer_id_validation.avsrc/test/resources/testdata/scripts/aviator/regex_match.avsrc/test/resources/testdata/scripts/aviator/return_numbers.avsrc/test/resources/testdata/scripts/aviator/return_string.avsrc/test/resources/testdata/scripts/aviator/simple_return_false.avsrc/test/resources/testdata/scripts/aviator/simple_return_true.avsrc/test/resources/testdata/scripts/aviator/upload_ratio_check.avwebui/src/views/custom-script/components/detailDrawer.vuewebui/src/views/custom-script/components/editor/index.vuewebui/src/views/custom-script/locale/en-US.tswebui/src/views/custom-script/locale/zh-CN.tswebui/src/views/custom-script/locale/zh-TW.ts
💤 Files with no reviewable changes (11)
- src/test/resources/testdata/scripts/aviator/ip_range_check.av
- src/test/resources/testdata/scripts/aviator/complex_condition.av
- src/test/resources/testdata/scripts/aviator/client_name_check.av
- src/test/resources/testdata/scripts/aviator/return_string.av
- src/test/resources/testdata/scripts/aviator/simple_return_true.av
- src/test/resources/testdata/scripts/aviator/peer_id_validation.av
- src/test/resources/testdata/scripts/aviator/invalid_syntax.av
- src/test/resources/testdata/scripts/aviator/upload_ratio_check.av
- src/test/resources/testdata/scripts/aviator/simple_return_false.av
- src/test/resources/testdata/scripts/aviator/regex_match.av
- src/test/resources/testdata/scripts/aviator/return_numbers.av
Deploying pbh-fe with
|
| Latest commit: |
0fae015
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://dc5fb025.peerbanhelper.pages.dev |
| Branch Preview URL: | https://reapply-python.peerbanhelper.pages.dev |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
webui/src/views/custom-script/components/detailDrawer.vue (1)
94-99:⚠️ Potential issue | 🟡 Minor新建脚本时缺少
form.fileType重置。当用户先查看/编辑
.py脚本后再创建新脚本，form.fileType会保留上一次的'py'值，导致新脚本默认类型错误。应在新建分支中将fileType重置为默认值'av'。🛠️ 建议修复
if (!id) { isNew.value = true form.name = '' + form.fileType = 'av' content.value = '' viewOnly.value = false } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webui/src/views/custom-script/components/detailDrawer.vue` around lines 94 - 99, In viewDetail (the function handling opening the drawer), the "new" branch fails to reset form.fileType so creating a new script inherits the previous fileType (e.g., 'py'); update the new-creation branch where isNew.value is set to true to also set form.fileType = 'av' (alongside form.name = '' and content.value = '' and viewOnly.value = false) so the default type for new scripts is correctly reset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyCompiledScript.kt`:
- Around line 26-34: The execute method on PyCompiledScript leaves shared
interpreter globals around and must explicitly isolate per-call state: inside
execute(...) (while holding interpreter?.lock) capture the current global names,
remove any keys not in the incoming env whitelist plus the two required names
("result" and "compiled_code") before setting env variables and running
exec(compiled_code), then after execution remove any globals that were
introduced by the script (i.e. keys that weren't present before the call); refer
to the execute method, interpreter (SharedInterpreter), compiledCode and env to
locate where to add the pre-cleaning and post-cleaning steps and perform all
changes under the existing lock.
In
`@src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyThreadSafeInterpreter.java`:
- Around line 115-120: shutdown() currently calls close() and
executor.shutdownNow() without taking the interpreter lock and does not handle
the checked JepException; modify PyThreadSafeInterpreter.shutdown() to acquire
the same lock used by PyCompiledScript.execute() (i.e., use interpreter.lock)
around the close() and executor.shutdownNow() sequence so shutdown shares the
execution critical section, and catch JepException from close() (log or rethrow
as unchecked) so the method compiles and exceptions are handled cleanly; ensure
you reference the interpreter.lock, close(), shutdown(), and
executor.shutdownNow() symbols when making the change.
---
Duplicate comments:
In `@webui/src/views/custom-script/components/detailDrawer.vue`:
- Around line 94-99: In viewDetail (the function handling opening the drawer),
the "new" branch fails to reset form.fileType so creating a new script inherits
the previous fileType (e.g., 'py'); update the new-creation branch where
isNew.value is set to true to also set form.fileType = 'av' (alongside form.name
= '' and content.value = '' and viewOnly.value = false) so the default type for
new scripts is correctly reset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c712f97-3df6-4aa2-b84b-c5c7bee1d36e
📒 Files selected for processing (8)
src/main/java/com/ghostchu/peerbanhelper/Main.javasrc/main/java/com/ghostchu/peerbanhelper/util/scriptengine/AVCompiledScript.javasrc/main/java/com/ghostchu/peerbanhelper/util/scriptengine/AVScriptEngine.javasrc/main/java/com/ghostchu/peerbanhelper/util/scriptengine/CompiledScript.ktsrc/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyCompiledScript.ktsrc/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyScriptEngine.ktsrc/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyThreadSafeInterpreter.javawebui/src/views/custom-script/components/detailDrawer.vue
💤 Files with no reviewable changes (1)
- src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/AVScriptEngine.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyScriptEngine.kt
- src/main/java/com/ghostchu/peerbanhelper/Main.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyCompiledScript.kt (2)
27-32: 可空解释器的use块内return语义需要注意。当
interpreter为null时，interpreter?.lock返回null，null.use {}不会执行 lambda 体，而是直接返回null。但use块内部的return语句实际上是从外部函数execute()返回。当
interpreter非空时，代码能正常工作。但建议添加显式的 null 检查以提高可读性：♻️ 可选重构建议
override fun execute(env: MutableMap<String, Any>): Any? { + val interp = interpreter ?: return null - interpreter?.lock.use { _ -> - interpreter?.set("compiled_code", compiledCode) + interp.lock.use { + interp.set("compiled_code", compiledCode) // ... rest of the code } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyCompiledScript.kt` around lines 27 - 32, The current execute() relies on interpreter?.lock.use { ... } with an internal return which is confusing when interpreter is null; add an explicit null check for interpreter at the start of execute() (e.g., if (interpreter == null) return null) and then call interpreter.lock.use { ... } with the body setting "compiled_code" and "env", executing the script and returning the value from interpreter.getValue("context.get('result')"); this ensures clear semantics for the nullable interpreter and avoids surprising control-flow from a nullable use block.
28-31: 全局状态隔离已基本解决，但解释器层面仍有变量残留。当前实现通过
context=dict(env)和exec(compiled_code, context)实现了脚本执行的隔离，脚本内部的变量不会污染后续执行，这解决了之前评审中提到的核心隔离问题。但
compiled_code、env、context等变量仍会残留在解释器的全局命名空间中。虽然这不会导致脚本间状态污染，但可能造成内存泄漏（如果 env 包含大对象）。可考虑在执行后清理这些变量。♻️ 建议在执行后清理临时变量
override fun execute(env: MutableMap<String, Any>): Any? { interpreter?.lock.use { _ -> interpreter?.set("compiled_code", compiledCode) interpreter?.set("env", env) - interpreter?.exec("context=dict(env)\n" + "exec(compiled_code,context)") - return interpreter?.getValue("context.get('result')") + interpreter?.exec(""" +_ctx=dict(env) +exec(compiled_code,_ctx) +_result=_ctx.get('result') +del compiled_code, env, _ctx +""".trimIndent()) + return interpreter?.getValue("_result") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyCompiledScript.kt` around lines 28 - 31, The interpreter retains temporary names (compiled_code, env, context) after exec; update PyCompiledScript.kt to explicitly remove those from the interpreter globals after retrieving the result (use the same interpreter instance that runs the script), e.g. call interpreter?.exec to del or unset "compiled_code", "env", and "context" (or use interpreter API to remove/reset those symbols) immediately after interpreter?.getValue("context.get('result')") to avoid retaining large objects in memory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyCompiledScript.kt`:
- Around line 26-33: The call interpreter?.getValue("context.get('result')") is
invalid because Jep's getValue only retrieves top-level variables, not method
invocations; change execute to retrieve the context object first (use
interpreter?.getValue("context") or similar) and then obtain the result by
invoking the mapping/get method on that object (e.g., call a get/invoke accessor
on the returned PyObject or call interpreter.invoke on a previously exposed
function) instead of embedding "context.get('result')" inside getValue; update
references around execute, interpreter, compiledCode and env accordingly.
---
Nitpick comments:
In
`@src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyCompiledScript.kt`:
- Around line 27-32: The current execute() relies on interpreter?.lock.use { ...
} with an internal return which is confusing when interpreter is null; add an
explicit null check for interpreter at the start of execute() (e.g., if
(interpreter == null) return null) and then call interpreter.lock.use { ... }
with the body setting "compiled_code" and "env", executing the script and
returning the value from interpreter.getValue("context.get('result')"); this
ensures clear semantics for the nullable interpreter and avoids surprising
control-flow from a nullable use block.
- Around line 28-31: The interpreter retains temporary names (compiled_code,
env, context) after exec; update PyCompiledScript.kt to explicitly remove those
from the interpreter globals after retrieving the result (use the same
interpreter instance that runs the script), e.g. call interpreter?.exec to del
or unset "compiled_code", "env", and "context" (or use interpreter API to
remove/reset those symbols) immediately after
interpreter?.getValue("context.get('result')") to avoid retaining large objects
in memory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7543e75-cc92-49af-ab9e-eeab8c0328f0
📒 Files selected for processing (2)
src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyCompiledScript.ktsrc/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyThreadSafeInterpreter.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyThreadSafeInterpreter.java (2)
151-159:Lock.close()声明throws Exception但实际不会抛出异常
unlock()不会抛出 checked exception，可以移除throws Exception声明以简化 API。♻️ 简化异常声明
`@Override` - public void close() throws Exception { + public void close() { lock.unlock(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyThreadSafeInterpreter.java` around lines 151 - 159, The Lock.close() method currently declares "throws Exception" but unlock() never throws a checked exception; remove the checked exception from the signature by changing the Lock.close declaration to a no-throws method (e.g., "public void close()") and keep the body calling lock.unlock(); update the Lock class (which implements AutoCloseable) accordingly so the API is simplified and callers don't need to handle a checked exception.
137-145:shutdown()中executor.shutdownNow()在锁释放后执行当前实现中，try-with-resources 会在
catch/finally之前释放锁。这意味着在锁释放和shutdownNow()之间存在一个小的时间窗口，其他线程可能提交新任务。如果这是预期行为（避免在持锁时等待 executor 关闭），建议添加注释说明。否则可考虑将
shutdownNow()移入 try 块内。💡 如果需要在持锁期间关闭 executor
public void shutdown() { try (final Lock _ = getLock()) { close(); + executor.shutdownNow(); } catch (Exception e) { throw new RuntimeException(e); - } finally { - executor.shutdownNow(); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyThreadSafeInterpreter.java` around lines 137 - 145, The shutdown() method currently releases the lock (via try-with-resources around getLock()) before calling executor.shutdownNow(), which allows other threads to submit tasks between lock release and shutdown; either move executor.shutdownNow() into the try block (so shutdownNow() runs while the lock from getLock() is held) or, if the current behavior is intentional, add a clear comment in shutdown() explaining that shutdownNow() is intentionally called after lock release to avoid holding the lock during executor shutdown; update references to getLock(), close(), and executor.shutdownNow() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyThreadSafeInterpreter.java`:
- Around line 28-39: The static init block in PyThreadSafeInterpreter spawns a
Process (using CHECK_SCRIPT) and a BufferedReader but doesn't close them or
guard against hangs; update the code around the Runtime.exec call so it uses
try-with-resources for the BufferedReader/InputStreamReader and obtains the
Process via ProcessBuilder (or keep the Process reference) then call
process.waitFor(timeout, TimeUnit.SECONDS) and if it times out call
process.destroyForcibly(), and always close streams (and call
process.getErrorStream()/drain or log it) before setting
MainInterpreter.setJepLibraryPath(ret); ensure exceptions are still caught and
logged but resources are reliably closed to avoid leaks and class-loading
blocking.
---
Nitpick comments:
In
`@src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyThreadSafeInterpreter.java`:
- Around line 151-159: The Lock.close() method currently declares "throws
Exception" but unlock() never throws a checked exception; remove the checked
exception from the signature by changing the Lock.close declaration to a
no-throws method (e.g., "public void close()") and keep the body calling
lock.unlock(); update the Lock class (which implements AutoCloseable)
accordingly so the API is simplified and callers don't need to handle a checked
exception.
- Around line 137-145: The shutdown() method currently releases the lock (via
try-with-resources around getLock()) before calling executor.shutdownNow(),
which allows other threads to submit tasks between lock release and shutdown;
either move executor.shutdownNow() into the try block (so shutdownNow() runs
while the lock from getLock() is held) or, if the current behavior is
intentional, add a clear comment in shutdown() explaining that shutdownNow() is
intentionally called after lock release to avoid holding the lock during
executor shutdown; update references to getLock(), close(), and
executor.shutdownNow() accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18dbd116-3461-461c-95a7-7f89e0c7b271
📒 Files selected for processing (1)
src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyThreadSafeInterpreter.java
Comment on lines
+28
to
+39
| try { | ||
| Process p = Runtime.getRuntime().exec(new String[]{"python" ,"-c", CHECK_SCRIPT}); | ||
| BufferedReader in = new BufferedReader(new InputStreamReader(p.getInputStream())); | ||
| String ret = in.readLine(); | ||
| if (ret != null && !ret.trim().isEmpty()) { | ||
| log.debug("Found libjep path: {}", ret); | ||
| MainInterpreter.setJepLibraryPath(ret); | ||
| } | ||
| } catch (Exception e) { | ||
| // 失败时保持静默 | ||
| log.debug("Failed to search libjep path", e); | ||
| } |
Contributor
There was a problem hiding this comment.
静态初始化块中存在资源泄漏和潜在阻塞风险
Process 和 BufferedReader 未正确关闭，会导致资源泄漏和潜在的僵尸进程。此外，没有设置超时机制，如果 Python 进程挂起，会导致类加载阻塞。
🔧 建议使用 try-with-resources 并添加超时
static {
// 设置 Jep jni 文件位置
try {
Process p = Runtime.getRuntime().exec(new String[]{"python" ,"-c", CHECK_SCRIPT});
- BufferedReader in = new BufferedReader(new InputStreamReader(p.getInputStream()));
- String ret = in.readLine();
- if (ret != null && !ret.trim().isEmpty()) {
- log.debug("Found libjep path: {}", ret);
- MainInterpreter.setJepLibraryPath(ret);
+ try {
+ boolean finished = p.waitFor(5, TimeUnit.SECONDS);
+ if (finished && p.exitValue() == 0) {
+ try (BufferedReader in = new BufferedReader(new InputStreamReader(p.getInputStream()))) {
+ String ret = in.readLine();
+ if (ret != null && !ret.trim().isEmpty()) {
+ log.debug("Found libjep path: {}", ret);
+ MainInterpreter.setJepLibraryPath(ret);
+ }
+ }
+ }
+ } finally {
+ p.destroyForcibly();
}
} catch (Exception e) {
// 失败时保持静默
log.debug("Failed to search libjep path", e);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/com/ghostchu/peerbanhelper/util/scriptengine/PyThreadSafeInterpreter.java`
around lines 28 - 39, The static init block in PyThreadSafeInterpreter spawns a
Process (using CHECK_SCRIPT) and a BufferedReader but doesn't close them or
guard against hangs; update the code around the Runtime.exec call so it uses
try-with-resources for the BufferedReader/InputStreamReader and obtains the
Process via ProcessBuilder (or keep the Process reference) then call
process.waitFor(timeout, TimeUnit.SECONDS) and if it times out call
process.destroyForcibly(), and always close streams (and call
process.getErrorStream()/drain or log it) before setting
MainInterpreter.setJepLibraryPath(ret); ensure exceptions are still caught and
logged but resources are reliably closed to avoid leaks and class-loading
blocking.
paulzzh
marked this pull request as draft
March 7, 2026 11:15
Collaborator
Author
|
感觉会带来的麻烦要更多，我又不觉得我们需要这玩意了） |
Member
|
我对性能感到担忧，Avaitor 好歹会把自己编译成字节码，吃上 Java JIT |
Collaborator
Author
其实 python 是会先编译成自己的字节码再解释的 |
Member
调用的 cpython 吗 |
Collaborator
Author
是 Jni 去找 cpython 但是我现在对这个脚本的格式不太满意，比如如果写了 |
Collaborator
Author
|
不想看了）仍需执行python二进制有点别扭）关了 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
外部 python3 实现
https://github.com/ninia/jep
pip install jeppip 也是编译安装，和
python setup.py build没区别，需要 jdk 25 和对应 python3 版本的头文件https://github.com/icemachined/jep-distro有第三方分享编译好的 wheel，但是很久不更新了，需要研究
测试：https://github.com/paulzzh/jep/releases ，docker 待研究
包体积非常小
Summary by CodeRabbit
New Features
Refactor
Chores