Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Worker_threads SharedArrayBuffer in multiple object properties cross-referenced randomly (object corruption?) #28559

Closed
eduardnegru opened this issue Jul 5, 2019 · 2 comments
Closed

Worker_threads SharedArrayBuffer in multiple object properties cross-referenced randomly (object corruption?) #28559

eduardnegru opened this issue Jul 5, 2019 · 2 comments
Labels
confirmed-bug Issues with confirmed bugs. worker Issues and PRs related to Worker support.

Comments

@eduardnegru
Copy link

eduardnegru commented Jul 5, 2019 •
edited

  • Version: 12.6.0
  • Platform: Linux dc 4.4.62-1.el7.elrepo.x86_64 deps: update openssl to 1.0.1j #1 SMP Tue Apr 18 11:02:11 EDT 2017 x86_64 x86_64 x86_64 GNU/Linux and 64-bit Windows
  • Subsystem: worker_threads

MainThread

const { Worker, isMainThread, parentPort, workerData } = require('worker_threads');

function allocateSharedMemory(objSharedBuffer, strBufferType, nBytesCount)
{
	objSharedBuffer[strBufferType] = new SharedArrayBuffer(nBytesCount);
	const arrSharedMemoryView = new Uint32Array(objSharedBuffer[strBufferType]);
	arrSharedMemoryView[0] = nBytesCount;
}

const arrFruit = [
	"apples",
	"bananas",
	"oranges",
];

const objToShare = {};

for(let strFruit of arrFruit)
{	
	objToShare[strFruit] = {};
	objToShare[strFruit]["type"] = {};
	
	objToShare[strFruit]["type"]["sunlight"] = {};
	
	allocateSharedMemory(objToShare[strFruit]["type"]["sunlight"], "lastHour", 100);
	allocateSharedMemory(objToShare[strFruit]["type"]["sunlight"], "lastDay", 104);
	allocateSharedMemory(objToShare[strFruit]["type"]["sunlight"], "lastMonth", 108);
	allocateSharedMemory(objToShare[strFruit]["type"]["sunlight"], "lastYear", 112);

	objToShare[strFruit]["type"]["water"] = {};

	allocateSharedMemory(objToShare[strFruit]["type"]["water"], "lastHour", 116);
	allocateSharedMemory(objToShare[strFruit]["type"]["water"], "lastDay", 120);
	allocateSharedMemory(objToShare[strFruit]["type"]["water"], "lastMonth", 124);
	allocateSharedMemory(objToShare[strFruit]["type"]["water"], "lastYear", 128);
}	

for(let i = 0; i < 4; i++)
{
	const w = new Worker("./WorkerThread.js");
	
	w.postMessage(objToShare);
	
	/*w.on('message', (msg) => {
		const arrDataPoints = new Uint32Array(objToShare["bananas"]["type"]["sunlight"]["lastHour"]);
	});*/	
}

WorkerThread

const {  parentPort, workerData } = require('worker_threads');

parentPort.once('message', (value) => {

	const objGranularities = {
		"lastHour": 100,
		"lastDay": 104,
		"lastMonth": 108,
		"lastYear": 112
	}

	for(let strGranularity in objGranularities)
	{
		const sharedBuffer = value["bananas"]["type"]["sunlight"][strGranularity];
		const sharedBufferView = new Uint32Array(sharedBuffer);
		const nByteLengthReceived = sharedBuffer.byteLength;
		const nByteLengthExpected = objGranularities[strGranularity]; 
		
		if(nByteLengthExpected === nByteLengthReceived)
		{
			console.log("[Pass]", "receivedLength =", nByteLengthReceived, "expectedLength =", nByteLengthExpected, "firstValue =", sharedBufferView[0], "type =", strGranularity);
			
		}
		else
		{
			console.log("[Fail]", "receivedLength =", nByteLengthReceived, "expectedLength =", nByteLengthExpected, "firstValue =", sharedBufferView[0],  "type =", strGranularity);
		}
		
	}

	parentPort.postMessage("finished");

});

Output

[Fail] receivedLength = 104 expectedLength = 100 firstValue = 104 type = lastHour
[Fail] receivedLength = 108 expectedLength = 104 firstValue = 108 type = lastDay
[Fail] receivedLength = 104 expectedLength = 100 firstValue = 104 type = lastHour
[Fail] receivedLength = 112 expectedLength = 108 firstValue = 112 type = lastMonth
[Fail] receivedLength = 108 expectedLength = 104 firstValue = 108 type = lastDay
[Fail] receivedLength = 128 expectedLength = 112 firstValue = 128 type = lastYear
[Fail] receivedLength = 112 expectedLength = 108 firstValue = 112 type = lastMonth
[Fail] receivedLength = 128 expectedLength = 112 firstValue = 128 type = lastYear
[Fail] receivedLength = 104 expectedLength = 100 firstValue = 104 type = lastHour
[Fail] receivedLength = 108 expectedLength = 104 firstValue = 108 type = lastDay
[Fail] receivedLength = 112 expectedLength = 108 firstValue = 112 type = lastMonth
[Fail] receivedLength = 128 expectedLength = 112 firstValue = 128 type = lastYear
[Fail] receivedLength = 104 expectedLength = 100 firstValue = 104 type = lastHour
[Fail] receivedLength = 108 expectedLength = 104 firstValue = 108 type = lastDay
[Fail] receivedLength = 112 expectedLength = 108 firstValue = 112 type = lastMonth
[Fail] receivedLength = 128 expectedLength = 112 firstValue = 128 type = lastYear

The order of creating the keys in the object matters.
It appears there's some optimization in V8 which is disabled partially (object keys reordering).

When ordered this way, everything works fine:

const arrFruit = [
	"bananas",
	"oranges",
	"apples",
];

{
  bananas: { type: { sunlight: [Object], water: [Object] } },
  oranges: { type: { sunlight: [Object], water: [Object] } },
  apples: { type: { sunlight: [Object], water: [Object] } }
}

I get the following output:

[Pass] receivedLength = 100 expectedLength = 100 firstValue = 100 type = lastHour
[Pass] receivedLength = 104 expectedLength = 104 firstValue = 104 type = lastDay
[Pass] receivedLength = 108 expectedLength = 108 firstValue = 108 type = lastMonth
[Pass] receivedLength = 112 expectedLength = 112 firstValue = 112 type = lastYear
[Pass] receivedLength = 100 expectedLength = 100 firstValue = 100 type = lastHour
[Pass] receivedLength = 104 expectedLength = 104 firstValue = 104 type = lastDay
[Pass] receivedLength = 100 expectedLength = 100 firstValue = 100 type = lastHour
[Pass] receivedLength = 108 expectedLength = 108 firstValue = 108 type = lastMonth
[Pass] receivedLength = 104 expectedLength = 104 firstValue = 104 type = lastDay
[Pass] receivedLength = 112 expectedLength = 112 firstValue = 112 type = lastYear
[Pass] receivedLength = 108 expectedLength = 108 firstValue = 108 type = lastMonth
[Pass] receivedLength = 112 expectedLength = 112 firstValue = 112 type = lastYear
[Pass] receivedLength = 100 expectedLength = 100 firstValue = 100 type = lastHour
[Pass] receivedLength = 104 expectedLength = 104 firstValue = 104 type = lastDay
[Pass] receivedLength = 108 expectedLength = 108 firstValue = 108 type = lastMonth
[Pass] receivedLength = 112 expectedLength = 112 firstValue = 112 type = lastYear
@eduardnegru eduardnegru changed the title SharedArrayBuffers are received in the wrong order inside workers. Worker_threads SharedArrayBuffer in multiple object properties cross-referenced randomly (object corruption?) Jul 5, 2019
@oxygen
Copy link

oxygen commented Jul 5, 2019 •
edited

I simplified this a bit.

MasterThread.js

const WorkerThread = require("worker_threads").Worker;

// Bug: the second object key (apples) 
// always gets the wrong SharedArrayBuffer instances in the wrong keys.
const objFruits_A = {
	oranges: {
		x212: new SharedArrayBuffer(212),
		x216: new SharedArrayBuffer(216),
		x222: new SharedArrayBuffer(222)
	},
	apples: {
		x100: new SharedArrayBuffer(100),
		x104: new SharedArrayBuffer(104),
		x108: new SharedArrayBuffer(108)
	}
};

// Bug: the second object key (oranges) 
// always gets the wrong SharedArrayBuffer instances in the wrong keys.
const objFruits_B = {
	apples: {
		x100: new SharedArrayBuffer(100),
		x104: new SharedArrayBuffer(104),
		x108: new SharedArrayBuffer(108)
	},
	oranges: {
		x212: new SharedArrayBuffer(212),
		x216: new SharedArrayBuffer(216),
		x222: new SharedArrayBuffer(222)
	}
};

const workerThread = new WorkerThread("./WorkerThread.js");
workerThread.postMessage(objFruits_A);
workerThread.postMessage(objFruits_B);

// The second key in the sent object seems to get the SharedArrayBuffer instances 
// shuffled somehow for both objFruits_A and objFruits_B.
setTimeout(process.exit, 3000);

WorkerThread.js:

const parentPort = require("worker_threads").parentPort;

parentPort.on("message", console.log);

Output:

C:\Users\oxygen\Desktop\SharedArrayBuffer>node --experimental-worker MainThread.js
{ oranges:
   { x212: SharedArrayBuffer { byteLength: 212 },
     x216: SharedArrayBuffer { byteLength: 216 },
     x222: SharedArrayBuffer { byteLength: 222 } },
  apples:
   { x100: SharedArrayBuffer { byteLength: 216 },
     x104: SharedArrayBuffer { byteLength: 222 },
     x108: SharedArrayBuffer { byteLength: 108 } } }
{ apples:
   { x100: SharedArrayBuffer { byteLength: 100 },
     x104: SharedArrayBuffer { byteLength: 104 },
     x108: SharedArrayBuffer { byteLength: 108 } },
  oranges:
   { x212: SharedArrayBuffer { byteLength: 104 },
     x216: SharedArrayBuffer { byteLength: 108 },
     x222: SharedArrayBuffer { byteLength: 222 } } }

@addaleax addaleax added confirmed-bug Issues with confirmed bugs. worker Issues and PRs related to Worker support. labels Jul 6, 2019
addaleax added a commit to addaleax/node that referenced this issue Jul 6, 2019
@addaleax
worker: fix passing multiple SharedArrayBuffers at once
992b38d 
V8 has a handle scope below each `GetSharedArrayBufferId()` call,
so using a `v8::Local` that outlives that handle scope to store
references to `SharedArrayBuffer`s is invalid and may cause accidental
de-duplication of passed `SharedArrayBuffer`s.

Use a persistent handle instead to address this issue.

Fixes: nodejs#28559
@addaleax addaleax mentioned this issue Jul 6, 2019
Closed
3 tasks
@addaleax
Copy link
Member

addaleax commented Jul 6, 2019

@eduardnegru Thanks for opening an issue about this (and thanks @oxygen for the simplified reproduction), #28582 should address the bug :)

@Trott Trott closed this as completed in db55c3c Jul 9, 2019
targos pushed a commit that referenced this issue Jul 20, 2019
@addaleax @targos
worker: fix passing multiple SharedArrayBuffers at once
0e2cbe6 
V8 has a handle scope below each `GetSharedArrayBufferId()` call,
so using a `v8::Local` that outlives that handle scope to store
references to `SharedArrayBuffer`s is invalid and may cause accidental
de-duplication of passed `SharedArrayBuffer`s.

Use a persistent handle instead to address this issue.

Fixes: #28559

PR-URL: #28582
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
confirmed-bug Issues with confirmed bugs. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

worker: fix passing multiple SharedArrayBuffers at once addaleax/node
3 participants
@addaleax @oxygen @eduardnegru