1 Commits

Author	SHA1	Message	Date
robobun	99c3824b31	fix(napi): Make cleanup hooks behavior match Node.js exactly (#21883) ... # Fix NAPI cleanup hook behavior to match Node.js This PR addresses critical differences in NAPI cleanup hook implementation that cause crashes when native modules attempt to remove cleanup hooks. The fixes ensure Bun's behavior matches Node.js exactly. ## Issues Fixed Fixes #20835 Fixes #18827 Fixes #21392 Fixes #21682 Fixes #13253 All these issues show crashes related to NAPI cleanup hook management: - #20835, #18827, #21392, #21682: Show "Attempted to remove a NAPI environment cleanup hook that had never been added" crashes with `napi_remove_env_cleanup_hook` - #13253: Shows `napi_remove_async_cleanup_hook` crashes in the stack trace during Vite dev server cleanup ## Key Behavioral Differences Addressed ### 1. Error Handling for Non-existent Hook Removal - **Node.js**: Silently ignores removal of non-existent hooks (see `node/src/cleanup_queue-inl.h:27-30`) - **Bun Before**: Crashes with `NAPI_PERISH` error - **Bun After**: Silently ignores, matching Node.js behavior ### 2. Duplicate Hook Prevention - **Node.js**: Uses `CHECK_EQ` which crashes in ALL builds when adding duplicate hooks (see `node/src/cleanup_queue-inl.h:24`) - **Bun Before**: Used debug-only assertions - **Bun After**: Uses `NAPI_RELEASE_ASSERT` to crash in all builds, matching Node.js ### 3. VM Termination Checks - **Node.js**: No VM termination checks in cleanup hook APIs - **Bun Before**: Had VM termination checks that could cause spurious failures - **Bun After**: Removed VM termination checks to match Node.js ### 4. Async Cleanup Hook Handle Validation - **Node.js**: Validates handle is not NULL before processing - **Bun Before**: Missing NULL handle validation - **Bun After**: Added proper NULL handle validation with `napi_invalid_arg` return ## Execution Order Verified Both Bun and Node.js execute cleanup hooks in LIFO order (Last In, First Out) as expected. ## Additional Architectural Differences Identified Two major architectural differences remain that affect compatibility but don't cause crashes: 1. **Queue Architecture**: Node.js uses a single unified queue for all cleanup hooks, while Bun uses separate queues for regular vs async cleanup hooks 2. **Iteration Safety**: Different behavior when hooks are added/removed during cleanup iteration These will be addressed in future work as they require more extensive architectural changes. ## Testing - Added comprehensive test suite covering all cleanup hook scenarios - Tests verify identical behavior between Bun and Node.js - Includes edge cases like duplicate hooks, non-existent removal, and execution order - All tests pass with the current fixes The changes ensure NAPI modules using cleanup hooks (like LMDB, native Rust modules, etc.) work reliably without crashes. --------- Co-authored-by: Claude Bot <claude-bot@bun.sh> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Kai Tamkun <kai@tamkun.io> Co-authored-by: Jarred Sumner <jarred@jarredsumner.com>	2025-08-15 21:08:53 -07:00