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.

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

benchmark: fix net/dgram.js infinite loop #486

Closed
wants to merge 1 commit into from
Closed

benchmark: fix net/dgram.js infinite loop #486

wants to merge 1 commit into from

Conversation

yosuke-furukawa
Copy link
Member

benchmark script is not finished. net/dgram.js has infinite loop.

@mscdex
Copy link
Contributor

mscdex commented Jan 17, 2015

I saw this the other day too, but I'm not sure what impact setImmediate() brings.

@bnoordhuis
Copy link
Member

I think there are two things going on here:

  1. the benchmark hasn't been updated after changes to the dgram binding interface
  2. using setImmediate() this way is an accidental fix; the return value from setImmediate() makes the socket.send() call compatible-ish with the new interface again

EDIT: Sorry, scratch that; I thought this benchmark was using the binding layer directly.

@yosuke-furukawa
Copy link
Member Author

if this bug is solved, all benchmark tests work fine.

I would like to ask a question. this callback seems to be called synchronously, is this a right behavior?

IMO, UDP does not check reaching the target, so the behavior looks fine.

@bnoordhuis
Copy link
Member

this callback seems to be called synchronously, is this a right behavior?

If that is what happens, then no, that would definitely be the wrong behavior. I don't think it's that, though. I did a quick check and it seems the callback is just never called. Will investigate.

@bnoordhuis
Copy link
Member

Okay, perhaps it does get called synchronously somehow.

@yosuke-furukawa
Copy link
Member Author

I investigate this issue using git bisect.

As a result, I suspect this commit (e49429e) occurs this issue ( deps: update libuv to v0.11.28 e49429e ).

@yosuke-furukawa
Copy link
Member Author

According to libuv CHANGELOG, I can find a commit " unix: try to write immediately in uv_udp_send libuv/libuv@4189122".

@bnoordhuis I guess this is the reason why socket.send callback is called synchronously. How do I fix this script? setImmediate is OK?

@yosuke-furukawa
Copy link
Member Author

ping @bnoordhuis ?

@@ -39,7 +39,7 @@ function server() {
function onsend() {
if (sent++ % num == 0)
for (var i = 0; i < num; i++)
socket.send(chunk, 0, chunk.length, PORT, '127.0.0.1', onsend);
socket.send(chunk, 0, chunk.length, PORT, '127.0.0.1', setImmediate(onsend));
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't it be like setImmediate.bind(null, onsend) ?

@bnoordhuis
Copy link
Member

@yosuke-furukawa I appreciate the fix for the test, but if callbacks are really getting called synchronously now, that will have to be addressed. It violates the 'callbacks are always asynchronous' contract.

@vkurchatkin
Copy link
Contributor

Closing. This PR doesn't seem to fix anything

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants