-
Notifications
You must be signed in to change notification settings - Fork 206
Qjs exit hook #959
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Qjs exit hook #959
Conversation
7d54d27 to
08bdad5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements the QuickJS exit hook functionality by adding support for njs.on('exit', callback) which allows JavaScript code to register exit hooks that are called when the context is destroyed.
Key changes include:
- Convert core class ID definitions from #define macros to enums for better type safety
- Implement a new
njsglobal object with exit hook functionality - Add test coverage for the exit hook feature
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/qjs.h | Converts class ID macros to enum and adds exit hook function declaration |
| src/qjs.c | Implements core exit hook functionality including the njs object and event handlers |
| nginx/t/js_exit.t | Adds new test file for HTTP exit hook functionality |
| nginx/t/stream_js_exit.t | Removes QuickJS skip condition to enable stream exit hook tests |
| nginx/ngx_stream_js_module.c | Refactors string handling to use JS_ToCStringLen directly |
| nginx/ngx_qjs_fetch.c | Updates string conversion calls to use pool-based allocation |
| nginx/ngx_js_shared_dict.c | Updates string handling for shared dictionary operations |
| nginx/ngx_js.h | Updates function signature and adds QuickJS conditional compilation |
| nginx/ngx_js.c | Integrates exit hook calls and updates string conversion implementation |
| nginx/ngx_http_js_module.c | Updates string conversion calls and fixes cleanup handler type |
| external/njs_shell.c | Adds exit hook call to engine destruction |
Comments suppressed due to low confidence (1)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
2797465 to
db255bc
Compare
VadimZhestikov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Previously in QuickJS engine, fields allocated from memory pool linked to QuickJS engine lifetime were stored in nginx data structs. This causes a heap-use-after-free if QuickJS engine is destroyed earlier than a last access from nginx. For example, it becomes visible when moving NJS cleanup handler from pool->cleanup to r->cleanup. The fix is to only store references in nginx objects allocated from nginx memory pool.
This closes nginx#955 issue on Github.
db255bc to
5abdc98
Compare
VadimZhestikov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
No description provided.