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

Removes FIXME #8689

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
7 participants
@ALJCepeda
Contributor

ALJCepeda commented Sep 21, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
Description of change

It's necessary to close the socket otherwise the test will hang
REF #4640

@lpinca

Please don't change file permissions.

@lpinca lpinca added the dgram label Sep 21, 2016

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Sep 21, 2016

Member

I feel like the comment is pointing to an underlying bug in the dgram impl if it should not be necessary.

Looks like it's been there since the test was introduced in ALJCepeda@defa637

Member

Fishrock123 commented Sep 21, 2016

I feel like the comment is pointing to an underlying bug in the dgram impl if it should not be necessary.

Looks like it's been there since the test was introduced in ALJCepeda@defa637

@ALJCepeda

This comment has been minimized.

Show comment
Hide comment
@ALJCepeda

ALJCepeda Sep 21, 2016

Contributor

My apologies! The mode change was not intentional, I've reverted back to 644 and ran git config core.fileMode false to prevent this from happening again in the future.

@Fishrock123 Yes that makes sense. I will investigate dgram further and see if I can come up with a more definitive answer. If it is a bug, should I create an issue before attempting to resolve it?

Contributor

ALJCepeda commented Sep 21, 2016

My apologies! The mode change was not intentional, I've reverted back to 644 and ran git config core.fileMode false to prevent this from happening again in the future.

@Fishrock123 Yes that makes sense. I will investigate dgram further and see if I can come up with a more definitive answer. If it is a bug, should I create an issue before attempting to resolve it?

@lpinca

lpinca approved these changes Sep 22, 2016

ALJCepeda added some commits Sep 21, 2016

Removes FIXME
It's necessary to close the socket otherwise the test will hang
Removes FIXME
It's necessary to close the socket otherwise the test will hang
@ALJCepeda

This comment has been minimized.

Show comment
Hide comment
@ALJCepeda

ALJCepeda Sep 22, 2016

Contributor

@Fishrock123 @Trott

When the first send call is made dgram will bind to port 0: https://github.com/nodejs/node/blob/master/lib/dgram.js#L349-L350

And start listening for messages by calling recvStart on its handle: https://github.com/nodejs/node/blob/master/lib/dgram.js#L110-L119

I think this is why you have to close the socket, otherwise the underlining handler stays bound. close calls recvStop on the handle: https://github.com/nodejs/node/blob/master/lib/dgram.js#L535

test-dgram-oob-buffer.js is not the only test that binds in this way:
https://github.com/nodejs/node/blob/master/test/parallel/test-dgram-close.js#L14-L15
https://github.com/nodejs/node/blob/master/test/sequential/test-dgram-pingpong.js#L36

However this seems unusual to me. test-dgram-oob-buffer.js will work without binding and that makes sense... it's only sending messages, not receiving any. I feel that rather than implicitly binding to the port on send it should be required that there's an explicit call to bind.

The other tests will work when I call bind

Thoughts?

Contributor

ALJCepeda commented Sep 22, 2016

@Fishrock123 @Trott

When the first send call is made dgram will bind to port 0: https://github.com/nodejs/node/blob/master/lib/dgram.js#L349-L350

And start listening for messages by calling recvStart on its handle: https://github.com/nodejs/node/blob/master/lib/dgram.js#L110-L119

I think this is why you have to close the socket, otherwise the underlining handler stays bound. close calls recvStop on the handle: https://github.com/nodejs/node/blob/master/lib/dgram.js#L535

test-dgram-oob-buffer.js is not the only test that binds in this way:
https://github.com/nodejs/node/blob/master/test/parallel/test-dgram-close.js#L14-L15
https://github.com/nodejs/node/blob/master/test/sequential/test-dgram-pingpong.js#L36

However this seems unusual to me. test-dgram-oob-buffer.js will work without binding and that makes sense... it's only sending messages, not receiving any. I feel that rather than implicitly binding to the port on send it should be required that there's an explicit call to bind.

The other tests will work when I call bind

Thoughts?

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Sep 22, 2016

Member

/cc @bnoordhuis I suppose...

Member

Trott commented Sep 22, 2016

/cc @bnoordhuis I suppose...

@cjihrig

cjihrig approved these changes Feb 8, 2017

I think this is fine. Once you start sending data, the socket becomes bound and needs to be closed.

@bnoordhuis

LGTM and sorry for the delay, I must've missed the notification.

Can you reword the commit log to something like 'test: remove obsolete comment from dgram test'? Thanks.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Feb 9, 2017

Contributor

Should be completely unnecessary, but CI: https://ci.nodejs.org/job/node-test-pull-request/6314/

Contributor

cjihrig commented Feb 9, 2017

Should be completely unnecessary, but CI: https://ci.nodejs.org/job/node-test-pull-request/6314/

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Feb 9, 2017

Contributor

Landed in 89f8ef2. Thanks!

Contributor

cjihrig commented Feb 9, 2017

Landed in 89f8ef2. Thanks!

@cjihrig cjihrig closed this Feb 9, 2017

cjihrig added a commit that referenced this pull request Feb 9, 2017

test: remove obsolete comment from dgram test
Refs: #4640
PR-URL: #8689
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 9, 2017

test: remove obsolete comment from dgram test
Refs: nodejs#4640
PR-URL: nodejs#8689
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

test: remove obsolete comment from dgram test
Refs: nodejs#4640
PR-URL: nodejs#8689
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017

test: remove obsolete comment from dgram test
Refs: nodejs#4640
PR-URL: nodejs#8689
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

jasnell added a commit that referenced this pull request Mar 7, 2017

test: remove obsolete comment from dgram test
Refs: #4640
PR-URL: #8689
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins added a commit that referenced this pull request Mar 9, 2017

test: remove obsolete comment from dgram test
Refs: #4640
PR-URL: #8689
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

@MylesBorins MylesBorins referenced this pull request Mar 9, 2017

Merged

v6.10.1 proposal #11759

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment