Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

don't close server socket if fcntl(O_NONBLOCK) throws EINVAL #1041

Merged
merged 2 commits into from
Jun 19, 2019
Merged

don't close server socket if fcntl(O_NONBLOCK) throws EINVAL #1041

merged 2 commits into from
Jun 19, 2019

Conversation

weissi
Copy link
Member

@weissi weissi commented Jun 18, 2019

Motivation:

Darwin does sometimes hand us back an (as of the docs) illegal error
code of EINVAL. So far, NIO has done the non-sensical thing of closing
the server socket as a result.

Modifications:

Just close the accepted socket, leave the server socket open, and fire
the error through the pipeline.

Result:

@weissi weissi requested a review from Lukasa June 18, 2019 17:43
@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Jun 18, 2019
@Lukasa Lukasa added this to the 2.3.0 milestone Jun 18, 2019
Lukasa
Lukasa requested changes Jun 18, 2019
View reviewed changes
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Generally in favour of fixing this, but I don’t love the specific approach here.

@weissi weissi added 🆕 semver/minor Adds new public API. and removed 🔨 semver/patch No public API change. labels Jun 18, 2019
@weissi weissi requested a review from Lukasa June 18, 2019 18:05
Lukasa
Lukasa requested changes Jun 19, 2019
View reviewed changes
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Cool, one tiny nit and then we're good.

@weissi
don't close server socket if fcntl(O_NONBLOCK) throws EINVAL
c01ca6e 
Motivation:

Darwin does sometimes hand us back an (as of the docs) illegal error
code of EINVAL. So far, NIO has done the non-sensical thing of closing
the server socket as a result.

Modifications:

Just close the accepted socket, leave the server socket open, and fire
the error through the pipeline.

Result:

- fixes apple#1030
- better behaviour
@weissi weissi requested a review from Lukasa June 19, 2019 13:22
@Lukasa Lukasa merged commit bc81605 into apple:master Jun 19, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@Lukasa Lukasa Lukasa approved these changes

Assignees
No one assigned
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.

ServerSocketChannel behaves non-sensical if fcntl on accepted socket returns EINVAL
2 participants
@weissi @Lukasa