Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

dgram: broadcast and multicast test fixes #2616

Closed

Conversation

wankdanker
Copy link

The broadcast and multicast multiple process tests don't fail properly when they are unable to receive messages or if the child processes die. This pull request fixes these shortcomings.

	- watch for the death of child processes and fail the test if they all die
	- use setTimeout to fail the test if responses are not received and processed in 5000ms
        - watch for the death of child processes and fail if they all die
        - use setTimeout to fail the test if responses are not received and processed in 5000ms
@AndreasMadsen
Copy link
Member

I'm a little concerned about using cluster this way (multicast adresses), when #2194 is fixed the testcases would property react very differently. As I see it we are relying on a bug.

@wankdanker
Copy link
Author

The current node v0.6 branch has a commit from libuv (b674187) which allows multiple processes to bind to the same UDP port. That should fix the issue seen in #2194.

The modifications in this pull request should allow us to better catch child processes dying because they were unable to bind to that port. The existing version of the tests would probably just sit waiting if the version of libuv in your branch does not include the reuse address fix.

So, are you saying that in the future, cluster will "proxy" for UDP the same way it does for TCP? In which case, yeah, we'd need to stop using cluster for these tests. Unless there is a way to turn off proxying for network streams.

@AndreasMadsen
Copy link
Member

Yes I'm concerned about that this testcase will fail or do something it is not supposed to, when #2194 has been fixed. bnoordhuis has confirmed that #2194 it is an issue, so I'm concluding that these testcases depend on the fact that UDP currently isn't supported by cluster.

To disable the unicast portsharing or proxy (or whatever we should call it) do not make any sense to me, since portsharing is the pure purpose of cluster. So I will sugess using child_process.fork().

You can just write something like:

if (process.argv[2] === 'child') {
  // this is in may ways equal to a worker
} else {
  // so this will be a master
  var workers = {};
  var fork = function () {
    var child = require('child_process').fork(process.argv[1], ['child']);
    workers[child.pid] = child;
    return child;
  }
  fork();
  fork();
}

@wankdanker
Copy link
Author

@AndreasMadsen : Cool. Thanks for the code. I see what you are saying. Even though all cluster workers may be able to bind to the same port with the SO_REUSEADDR fix, we need cluster to balance the load among the workers. So cluster will be handling UDP in the future.

I will modify the tests to use child processes the way you have suggested.

@AndreasMadsen
Copy link
Member

If I'm not mistaken this has landed in a0119af, but @isaacs forgot to close the issue :)

@koichik
Copy link

koichik commented Jan 27, 2012

@AndreasMadsen - Yeah, closing.

@wankdanker - Thanks for your contribution!

@koichik koichik closed this Jan 27, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants