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

Fixes setting IO blocking on OpenBSD #5283

Merged

Conversation

wmoxam
Copy link
Contributor

@wmoxam wmoxam commented Nov 12, 2017

Fixes #4997

It's worth noting that this issue is what prevents the crystal deps command from working on OpenBSD

@RX14
Copy link
Contributor

RX14 commented Nov 12, 2017

Why should this be necessary? Surely this breaks the principle of least surprise. I'd love to hear from someone working on openbsd why this doesn't work.

@jhass
Copy link
Member

jhass commented Nov 12, 2017

Do you have any insight as to why calling fcntl(F_SETFL, fcntl(F_GETFL)) is problematic?

At least there should be a comment explaining that is necessary for OpenBSD support, ideally also stating why.

@RX14
Copy link
Contributor

RX14 commented Nov 12, 2017

Perhaps try to reproduce in C and ask the mailinglist.

@wmoxam
Copy link
Contributor Author

wmoxam commented Nov 12, 2017

If I remember correctly this is only problematic in only one particular state, so perhaps my fix is too broad.

I'll add some test cases and produce a more concise fix

@wmoxam
Copy link
Contributor Author

wmoxam commented Nov 17, 2017

For reference, Ruby made a similar change because F_SETFL is an expensive operation since it needs to affect all processes with the same file object

https://bugs.ruby-lang.org/issues/4528

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

Looks good to me. Let's avoid a useless syscall when O_NONBLOCK is already set/unset.

@RX14
Copy link
Contributor

RX14 commented Nov 18, 2017

@ysbaddaden this is a fine performance optimization, but it shouldn't be necessary to run the program.

@RX14
Copy link
Contributor

RX14 commented Nov 18, 2017

It would be great to get some more investigation, but this is fine to merge.

@RX14 RX14 added this to the Next milestone Nov 18, 2017
@RX14 RX14 merged commit 47f7e35 into crystal-lang:master Nov 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants