-
-
Notifications
You must be signed in to change notification settings - Fork 27
feat: add AbortSignal support to all pdf* functions
#742
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
Conversation
Co-authored-by: Fdawgs <[email protected]>
Co-authored-by: Fdawgs <[email protected]>
Co-authored-by: Fdawgs <[email protected]>
…tency Co-authored-by: Fdawgs <[email protected]>
|
@copilot unit tests on windows 24 are failing. |
Co-authored-by: Fdawgs <[email protected]>
Fixed in ed8c3ec. The test was using
Fixed in commit 99ef784 by using Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…s reliability Co-authored-by: Fdawgs <[email protected]>
pdf* functions
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 adds AbortSignal support to all pdf* functions in the node-poppler library, enabling users to cancel in-progress PDF operations. The implementation follows the pattern from node-unrtf#536 and includes proper race condition handling.
Key Changes
- Added
PopplerExtraOptionstypedef with optionalsignalproperty - Updated all 11
pdf*functions to accept anextrasparameter withAbortSignalsupport - Implemented race condition prevention using
errorHandledflag in bothexecBinaryandpdfInfofunctions - Added comprehensive abort tests for
pdfAttachandpdfInfofunctions
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/index.js | Added PopplerExtraOptions typedef, updated execBinary and pdfInfo with error handling, and added extras parameter with signal support to all 11 pdf* functions |
| test/index.test.js | Added abort signal tests for pdfAttach and pdfInfo, covering pre-abort, during-abort, and non-abort scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/index.test.js
Outdated
| } | ||
|
|
||
| if (abortDuring) { | ||
| setTimeout(() => controller.abort(), 10); |
Copilot
AI
Jan 8, 2026
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.
Inconsistent abort timing mechanism compared to pdfInfo tests. The pdfInfo tests use setImmediate to abort on the next tick (line 590), which ensures spawn has time to start and avoids race conditions. For consistency and to prevent potential race conditions on different platforms or Node versions, this test should also use setImmediate instead of setTimeout with a hardcoded delay.
| setTimeout(() => controller.abort(), 10); | |
| setImmediate(() => controller.abort()); |
Original prompt
pdf*functions #741✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.