Repository navigation
Crash deserializing IPC message using advanced serialization #34797
Description
- Version: 14.8.0
- Platform: MacOS
- Subsystem: child_process
What steps will reproduce the bug?
I start a child process using fork() and { serialization: 'advanced' }. In the child process I synchronously call process.send() until it returns false. I keep process.channel referenced until all my send() callbacks have been called. Every so often, the main process crashes. Other times it exits gracefully, though it does not log all received messages. Presumably because the child process exits without flushing its IPC buffer. That's manageable and probably not a bug.
main.js:
const childProcess = require('child_process')
const channel = childProcess.fork('./child.js', [], { serialization: 'advanced' })
channel.on('message', message => {
console.error('main:', message.count)
})child.js:
// Keep the process alive until all messages have been sent.
process.channel.ref()
let pending = 0
const drain = () => {
if (--pending === 0) {
console.error('all messages sent')
if (!process.connected) {
console.error('already disconnected')
} else {
console.error('unref channel')
// Allow the process to exit.
process.channel.unref()
}
}
}
// Fill up any internal buffers.
const filler = Buffer.alloc(2 ** 12, 1)
// Enqueue as many messages as possible until we're told to back off.
let count = 0
let ok
do {
pending++
ok = process.send({count: ++count, filler}, drain)
console.error('child:', count, ok)
} while (ok)And then run node main.js.
How often does it reproduce? Is there a required condition?
It's intermittent.
What is the expected behavior?
The main process does not crash.
What do you see instead?
The main process crashes with the following error:
internal/child_process/serialization.js:69
deserializer.readHeader();
^
Error: Unable to deserialize cloned data due to invalid or unsupported version.
at parseChannelMessages (internal/child_process/serialization.js:69:20)
at parseChannelMessages.next (<anonymous>)
at Pipe.channel.onread (internal/child_process.js:595:18)
Additional information
If I modify child.process to schedule sends using setImmediate() there is no crash, and the main process receives all messages:
let count = 0
do {
pending++
setImmediate(() => {
process.send({count: ++count, filler}, drain)
console.error('child:', count)
})
} while (pending < 100)
Activity
Buffer IPC sends to avoid crashes
targos commentedon Aug 16, 2020
@nodejs/workers
Use advanced serialization (when available) for worker communication
addaleax commentedon Aug 17, 2020
Fwiw, I can’t seem to reproduce this locally. (And it’s more of a case for @nodejs/child_process, if we go by teams.)
targos commentedon Aug 17, 2020
Yeah, sorry. I initially thought it was maybe a general issue with our implementation of the serialization API and AFAIK there's no team for the
v8module.Now that I'm looking again at the issue, I wonder if it may be related to the use of different incompatible Node.js/V8 versions between parent and child processes.
addaleax commentedon Aug 17, 2020
I think that would lead to more consistent failures? My best guess would be that we somehow mess up the message boundaries and start parsing in the middle of another message, but I’m not really sure how that would happen…
novemberborn commentedon Aug 17, 2020
I'm using
fork()so it should select the same version. It feels like a data corruption issue to me.novemberborn commentedon Aug 17, 2020
When I was digging into the JS code that drives this there's a bunch of array buffers, so maybe the copying of data off the channel is corrupting a shared buffer? (I'm just thinking out loud, not really sure what the code is doing.)
addaleax commentedon Aug 17, 2020
@novemberborn There aren’t any SharedArrayBuffer instances involved, if that’s what you’re referring to.
The format for messages is relatively simple: It’s 4 bytes that contain the length of the message, plus the rest of the message. We use that first field to determine the message boundaries, so if that calculation goes wrong at some point, that might be the cause of a crash like this (the code does look okay to me, though). Another possibility would be that data is indeed corrupted during transfer, but I’m not sure how that would happen either.
If you’re up for building Node.js to debug this, you could probably print the contents of the
Buffers on the sending and the receiving side to compare them (or at least that’s what I would do if I could reproduce this).novemberborn commentedon Aug 17, 2020
@addaleax what OS are you using? #30009 also hints at a macOS issue, though that example isn't respecting backpressure.
addaleax commentedon Aug 17, 2020
I’m on x64 Linux, so it’s definitely possible that this is platform-specific, yes.
lpinca commentedon Aug 17, 2020
I can reproduce described issue on macOS 10.15.6
lpinca commentedon Aug 17, 2020
FWIW it seems to be fixed on master.
10 remaining items
KishanBagaria commentedon May 19, 2021
+1 we're using electron 11 / node 12
would be great to have this back-ported!
addaleax commentedon May 23, 2021
@peeter-tomberg @KishanBagaria It should follow the regular release schedule, so, yes, that should not be an issue.
child_process: retain reference to data with advanced serialization
child_process: retain reference to data with advanced serialization
child_process: retain reference to data with advanced serialization
child_process: retain reference to data with advanced serialization
child_process: retain reference to data with advanced serialization
Fix CI: Use Node.js 22 for Full Check job to resolve flaky test runner
Fix CI: Use Node.js 22 for Full Check job to resolve flaky test runner
Fix CI: Use Node.js 22 for Full Check job to resolve flaky test runner
Fix CI: Add retry mechanism for flaky test runner
Fix: Add retry logic to test job to handle flaky session-tracker test
child_process: retain reference to data with advanced serialization