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 #23507 sockets are blocking by default, this needs to reflect that #8630

Merged
merged 1 commit into from
Nov 25, 2022

Conversation

adamdruppe
Copy link
Contributor

This is remarkable to me - git blame says this was there in the initial commit to git in 2007, meaning it goes back to prehistory (though I could probably download some ancient phobos and check there, but i suspect it has been there the whole time), but I'm almost certain this is wrong.

Windows sockets are blocking by default. The Phobos docs, the constructor for Socket, states that it "creates a blocking socket". The behavior is blocking. But this property defaults to false.... and a new commit to my code happened to use it, assuming it had correct info, causing a regression for me!

But, as far as I can tell, this should have been true by default the whole time.

@dlang-bot
Copy link
Contributor

dlang-bot commented Nov 24, 2022

Thanks for your pull request and interest in making D better, @adamdruppe! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
23507 normal Socket.blocking property incorrect on new Socket on Windows

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8630"

@thewilsonator thewilsonator added the Review:Needs Changelog A changelog entry needs to be added to /changelog label Nov 25, 2022
@thewilsonator
Copy link
Contributor

I feel that this should have a changelog entry so there is documentation of this.

@adamdruppe
Copy link
Contributor Author

i just filed a bugzilla https://issues.dlang.org/show_bug.cgi?id=23507

@adamdruppe adamdruppe changed the title sockets are blocking by default, this needs to reflect that fixes #23507 sockets are blocking by default, this needs to reflect that Nov 25, 2022
@thewilsonator
Copy link
Contributor

Need to update the commit title, otherwise this looks good to go. I don't think a test is really necessary, but I won't say no to one.

@thewilsonator thewilsonator removed the Review:Needs Changelog A changelog entry needs to be added to /changelog label Nov 25, 2022
@adamdruppe
Copy link
Contributor Author

I don't think a test can be meaningful here unless it actually maybe asked the operating system to confirm the state which didn't used to be possible. I think it is now but it is a bit of a hassle.

@dlang-bot dlang-bot merged commit f710b08 into dlang:master Nov 25, 2022
@skoppe
Copy link

skoppe commented Nov 25, 2022

What if you create a Socket by passing in an existing socket_t? https://github.com/adamdruppe/phobos/blob/6fa48e418e8bf134dd4cf07bbf3900eb57305788/std/socket.d#L2733

It might be blocking, non-blocking, who knows? Well, on linux we can check, but on windows there doesn't seem to be an api for it (says 5min googling).

Still, this MR should go in, because they do start out blocking.

@adamdruppe
Copy link
Contributor Author

Yeah, we should probably at least document that... or make it a required argument to that ctor but that's a hassle.

i kinda like std.socket but i also kinda hate it too lol it is useful but has a few rough edges

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.

4 participants