Skip to content

Conversation

@xeioex
Copy link
Contributor

@xeioex xeioex commented Aug 27, 2025

No description provided.

@xeioex xeioex force-pushed the qjs_exit_hook branch 3 times, most recently from 7d54d27 to 08bdad5 Compare August 27, 2025 06:27
Copy link

Copilot AI left a 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 njs global 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.

VadimZhestikov
VadimZhestikov previously approved these changes Sep 8, 2025
Copy link
Contributor

@VadimZhestikov VadimZhestikov left a 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.
Copy link
Contributor

@VadimZhestikov VadimZhestikov left a comment

Choose a reason for hiding this comment

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

Looks good

@xeioex xeioex merged commit 04f6dfb into nginx:master Sep 8, 2025
2 checks passed
@xeioex xeioex deleted the qjs_exit_hook branch September 8, 2025 22:59
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.

2 participants