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

test: dynamic port in cluster worker dgram #12487

Closed
wants to merge 1 commit into from
Closed

test: dynamic port in cluster worker dgram #12487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions test/parallel/test-cluster-dgram-1.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ function master() {
cluster.fork();

// Wait until all workers are listening.
cluster.on('listening', common.mustCall(() => {
cluster.on('listening', common.mustCall((worker, address) => {
if (++listening < NUM_WORKERS)
return;

Expand All @@ -60,7 +60,7 @@ function master() {
doSend();

function doSend() {
socket.send(buf, 0, buf.length, common.PORT, '127.0.0.1', afterSend);
socket.send(buf, 0, buf.length, address.port, address.address, afterSend);
}

function afterSend() {
Expand Down Expand Up @@ -111,5 +111,5 @@ function worker() {
}
}, PACKETS_PER_WORKER));

socket.bind(common.PORT);
socket.bind(0);
Copy link
Member

@Trott Trott Apr 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is right. I think each of the four workers will get a different port with this change. In the current test, they are all using the same port and I think that behavior should be preserved. Otherwise, all the messages go to a single worker.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Trott I think they'll be using the same port because exclusive defaults to false and the fd will be shared among workers

Copy link
Member

@Trott Trott Apr 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@santigimeno Debugging indicates you are correct. Clearing my review. Thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@santigimeno sorry, but mind explaining why this is correct?

Copy link
Member

@santigimeno santigimeno Apr 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benjamingr, from the dgram documentation:

The options object may contain an additional exclusive property that is use when using dgram.Socket objects with the cluster module. When exclusive is set to false (the default), cluster workers will use the same underlying socket handle allowing connection handling duties to be shared. When exclusive is true, however, the handle is not shared and attempted port sharing results in an error.

I think this is how it works:

When a worker binds a UDP socket in non-exclusive mode, it requests the master process for a valid socket handle by sending a queryServer cluster message with the socket info (key). The master will create the socket handle only after receiving the first message with that specific key and then send the handle back to the worker. The following messages from the workers requesting a socket handle with the same key, will cause the master to send the same handle back to every worker.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@santigimeno thanks!

}
34 changes: 21 additions & 13 deletions test/parallel/test-cluster-dgram-2.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ const PACKETS_PER_WORKER = 10;

const cluster = require('cluster');
const dgram = require('dgram');
const assert = require('assert');


if (common.isWindows) {
Expand All @@ -45,7 +46,14 @@ function master() {

// Start listening on a socket.
const socket = dgram.createSocket('udp4');
socket.bind(common.PORT);
socket.bind({ port: 0 }, common.mustCall(() => {

// Fork workers.
for (let i = 0; i < NUM_WORKERS; i++) {
const worker = cluster.fork();
worker.send({ port: socket.address().port });
}
}));

// Disconnect workers when the expected number of messages have been
// received.
Expand All @@ -61,10 +69,6 @@ function master() {
cluster.disconnect();
}
}, NUM_WORKERS * PACKETS_PER_WORKER));

// Fork workers.
for (let i = 0; i < NUM_WORKERS; i++)
cluster.fork();
}


Expand All @@ -78,13 +82,17 @@ function worker() {
// send(), explicitly bind them to an ephemeral port.
socket.bind(0);

// There is no guarantee that a sent dgram packet will be received so keep
// sending until disconnect.
const interval = setInterval(() => {
socket.send(buf, 0, buf.length, common.PORT, '127.0.0.1');
}, 1);
process.on('message', common.mustCall((msg) => {
assert(msg.port);

// There is no guarantee that a sent dgram packet will be received so keep
// sending until disconnect.
const interval = setInterval(() => {
socket.send(buf, 0, buf.length, msg.port, '127.0.0.1');
}, 1);

cluster.worker.on('disconnect', () => {
clearInterval(interval);
});
cluster.worker.on('disconnect', () => {
clearInterval(interval);
});
}));
}