fix(napi): copy async task input on the JS thread for soundness
Review feedback on #337: holding the napi Buffer and reading it from the libuv worker was unsound. Buffer derefs straight to the JS-side allocation, so a caller mutating it before the promise settled would race the worker's reads — undefined behavior, not a recoverable error, and the documented don't-mutate contract was unenforceable. Deferring the copy to compute() would not help: any off-thread read races the same way. The JS thread is the only race-free place to take the copy, because JS is single-threaded and nothing can mutate the buffer during the synchronous part of the call. Revert to an owned Vec<u8> copied at call time. The cost is one memcpy, negligible next to the parse the async variants exist to unblock. Docs now state the buffer may be reused or mutated immediately, and a test locks in the copy semantics by mutating the input while a parse is in flight. Co-authored-by: Abimael Martell <abimaelmartell@users.noreply.github.com>
This commit is contained in:
co-authored by
Abimael Martell
parent
74a633dfd0
commit
92e2d6ec8e
+1
-1
@@ -87,7 +87,7 @@ for (const region of result[0].regions) {
|
||||
|
||||
`processPdf`, `classifyPdf`, and `extractPagesMarkdown` are synchronous and parse on the calling thread — in Node, that's the event loop. For a one-off call in a script that's fine, but in a server a large document can hold the loop for tens to hundreds of milliseconds.
|
||||
|
||||
`processPdfAsync`, `classifyPdfAsync`, and `extractPagesMarkdownAsync` take the same arguments and produce the same results, but run the parse on the libuv thread pool and return a promise, keeping the event loop free. The input buffer is read in place — no copy is made, so don't mutate it until the promise settles:
|
||||
`processPdfAsync`, `classifyPdfAsync`, and `extractPagesMarkdownAsync` take the same arguments and produce the same results, but run the parse on the libuv thread pool and return a promise, keeping the event loop free. The input buffer is copied before the call returns, so it's safe to reuse or mutate immediately:
|
||||
|
||||
```typescript
|
||||
import { classifyPdfAsync, extractPagesMarkdownAsync } from '@firecrawl/pdf-inspector'
|
||||
|
||||
+36
-26
@@ -715,16 +715,18 @@ fn to_page_region_texts(results: Vec<pdf_inspector::PageRegionResult>) -> Vec<Pa
|
||||
// concurrent load keep answering requests while a document parses. The sync
|
||||
// exports keep their names, signatures, and behaviour.
|
||||
//
|
||||
// The tasks hold the napi `Buffer` itself rather than a copy: the Buffer
|
||||
// keeps a reference that pins the JS-side allocation for the task's lifetime,
|
||||
// and its backing store is stable, so `compute` can read it from the worker
|
||||
// thread without an event-loop-blocking memcpy at call time. The caller must
|
||||
// not mutate the buffer until the promise settles — the same contract as
|
||||
// Node's own async `fs` APIs.
|
||||
// Each factory copies the input Buffer to an owned `Vec<u8>` on the calling
|
||||
// (JS) thread — deliberately. JS execution is single-threaded, so no JS code
|
||||
// can mutate the buffer while the synchronous part of the call copies it.
|
||||
// Holding the napi `Buffer` and reading it from the worker instead would be
|
||||
// zero-copy, but a caller mutating the buffer before the promise settles
|
||||
// would then race the worker's reads — undefined behavior, not a recoverable
|
||||
// error (a known napi-rs soundness hazard with cross-thread Buffer access).
|
||||
// The copy is a one-time memcpy, negligible next to the parse it unblocks.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
pub struct ProcessPdfTask {
|
||||
buffer: Buffer,
|
||||
bytes: Vec<u8>,
|
||||
pages: Option<Vec<u32>>,
|
||||
}
|
||||
|
||||
@@ -733,13 +735,13 @@ impl Task for ProcessPdfTask {
|
||||
type JsValue = PdfResult;
|
||||
|
||||
fn compute(&mut self) -> Result<Self::Output> {
|
||||
let bytes: &[u8] = &self.buffer;
|
||||
let bytes = std::mem::take(&mut self.bytes);
|
||||
let pages = self.pages.take();
|
||||
// AssertUnwindSafe: the closure only reads `bytes` and owns `pages`;
|
||||
// on unwind no shared state can be observed broken.
|
||||
// AssertUnwindSafe: `bytes`/`pages` are moved into the closure and
|
||||
// dropped on unwind — no shared state can be observed broken.
|
||||
catch_panic(
|
||||
"process_pdf",
|
||||
panic::AssertUnwindSafe(move || process_pdf_impl(bytes, pages)),
|
||||
panic::AssertUnwindSafe(move || process_pdf_impl(&bytes, pages)),
|
||||
)
|
||||
}
|
||||
|
||||
@@ -750,17 +752,20 @@ impl Task for ProcessPdfTask {
|
||||
|
||||
/// Async variant of [`processPdf`]: same result, but the parse runs on the
|
||||
/// libuv thread pool instead of the event loop and the call returns a
|
||||
/// promise. The buffer is read in place (no copy) — do not mutate it until
|
||||
/// the promise settles.
|
||||
/// promise. The buffer is copied before the call returns, so it may be
|
||||
/// reused or mutated immediately.
|
||||
// ts_return_type is required: napi-rs emits `Promise<unknown>` for
|
||||
// `AsyncTask<T>` returns without it.
|
||||
#[napi(ts_return_type = "Promise<PdfResult>")]
|
||||
pub fn process_pdf_async(buffer: Buffer, pages: Option<Vec<u32>>) -> AsyncTask<ProcessPdfTask> {
|
||||
AsyncTask::new(ProcessPdfTask { buffer, pages })
|
||||
AsyncTask::new(ProcessPdfTask {
|
||||
bytes: buffer.to_vec(),
|
||||
pages,
|
||||
})
|
||||
}
|
||||
|
||||
pub struct ClassifyPdfTask {
|
||||
buffer: Buffer,
|
||||
bytes: Vec<u8>,
|
||||
}
|
||||
|
||||
impl Task for ClassifyPdfTask {
|
||||
@@ -768,10 +773,10 @@ impl Task for ClassifyPdfTask {
|
||||
type JsValue = PdfClassification;
|
||||
|
||||
fn compute(&mut self) -> Result<Self::Output> {
|
||||
let bytes: &[u8] = &self.buffer;
|
||||
let bytes = std::mem::take(&mut self.bytes);
|
||||
catch_panic(
|
||||
"classify_pdf",
|
||||
panic::AssertUnwindSafe(move || classify_pdf_impl(bytes)),
|
||||
panic::AssertUnwindSafe(move || classify_pdf_impl(&bytes)),
|
||||
)
|
||||
}
|
||||
|
||||
@@ -782,15 +787,17 @@ impl Task for ClassifyPdfTask {
|
||||
|
||||
/// Async variant of [`classifyPdf`]: same result, but the classification runs
|
||||
/// on the libuv thread pool instead of the event loop and the call returns a
|
||||
/// promise. The buffer is read in place (no copy) — do not mutate it until
|
||||
/// the promise settles.
|
||||
/// promise. The buffer is copied before the call returns, so it may be
|
||||
/// reused or mutated immediately.
|
||||
#[napi(ts_return_type = "Promise<PdfClassification>")]
|
||||
pub fn classify_pdf_async(buffer: Buffer) -> AsyncTask<ClassifyPdfTask> {
|
||||
AsyncTask::new(ClassifyPdfTask { buffer })
|
||||
AsyncTask::new(ClassifyPdfTask {
|
||||
bytes: buffer.to_vec(),
|
||||
})
|
||||
}
|
||||
|
||||
pub struct ExtractPagesMarkdownTask {
|
||||
buffer: Buffer,
|
||||
bytes: Vec<u8>,
|
||||
pages: Option<Vec<u32>>,
|
||||
}
|
||||
|
||||
@@ -799,11 +806,11 @@ impl Task for ExtractPagesMarkdownTask {
|
||||
type JsValue = PagesExtractionResult;
|
||||
|
||||
fn compute(&mut self) -> Result<Self::Output> {
|
||||
let bytes: &[u8] = &self.buffer;
|
||||
let bytes = std::mem::take(&mut self.bytes);
|
||||
let pages = self.pages.take();
|
||||
catch_panic(
|
||||
"extract_pages_markdown",
|
||||
panic::AssertUnwindSafe(move || extract_pages_markdown_impl(bytes, pages.as_deref())),
|
||||
panic::AssertUnwindSafe(move || extract_pages_markdown_impl(&bytes, pages.as_deref())),
|
||||
)
|
||||
}
|
||||
|
||||
@@ -814,12 +821,15 @@ impl Task for ExtractPagesMarkdownTask {
|
||||
|
||||
/// Async variant of [`extractPagesMarkdown`]: same result, but the extraction
|
||||
/// runs on the libuv thread pool instead of the event loop and the call
|
||||
/// returns a promise. The buffer is read in place (no copy) — do not mutate
|
||||
/// it until the promise settles.
|
||||
/// returns a promise. The buffer is copied before the call returns, so it
|
||||
/// may be reused or mutated immediately.
|
||||
#[napi(ts_return_type = "Promise<PagesExtractionResult>")]
|
||||
pub fn extract_pages_markdown_async(
|
||||
buffer: Buffer,
|
||||
pages: Option<Vec<u32>>,
|
||||
) -> AsyncTask<ExtractPagesMarkdownTask> {
|
||||
AsyncTask::new(ExtractPagesMarkdownTask { buffer, pages })
|
||||
AsyncTask::new(ExtractPagesMarkdownTask {
|
||||
bytes: buffer.to_vec(),
|
||||
pages,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -169,6 +169,15 @@ assert.equal(asyncPicked.pages[0].page, 2);
|
||||
assert.equal(asyncPicked.pages[1].page, 0);
|
||||
console.log(' extractPagesMarkdownAsync with pages: OK');
|
||||
|
||||
// input buffer is copied at call time: mutating it immediately after the
|
||||
// call must not affect the in-flight parse
|
||||
const scratch = Buffer.from(fixture);
|
||||
const inFlight = processPdfAsync(scratch);
|
||||
scratch.fill(0);
|
||||
const fromMutated = await inFlight;
|
||||
assert.equal(fromMutated.markdown, result.markdown);
|
||||
console.log(' processPdfAsync input copied at call time: OK');
|
||||
|
||||
// concurrent async calls all settle
|
||||
const [c1, c2, c3] = await Promise.all([
|
||||
processPdfAsync(fixture),
|
||||
|
||||
Reference in New Issue
Block a user