-
Notifications
You must be signed in to change notification settings - Fork 216
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
Set close-on-exec and similar by default. #117
Conversation
Note that this PR doesn't re-add the workaround for missing SOCK_CLOEXEC support, in line with rust-lang/rust#74606. That keeps the code much simpler. |
I'm not sure about the |
Some alternatives I can think of: Although out of all of those, |
I don't want to accept this as is. With I'll change |
That's not true though, at-least not without putting more It's also not a fix for
It's still only calling a single syscall, just with the correct platform specific flags. That seems fine for a /edit: Except for apple targets I suppose, but it's still performing platform specific actions. |
@de-vri-es I created pr #125 for |
The relevant commit is fdc4fd4. |
I'm having a hard time contributing this way. I can understand feedback and requests for changes, but now you're leaving my PR open without feedback for 11 days and then break my PR without letting me implement feedback. I would appreciate a more contribution friendly workflow :( |
I'm sorry about this. As I said I wanted to this think about the pr (and the bigger picture of flags set to et etc.) a bit more, but I've been rather busy and socket2 is simply not among my highest priorities. This is even further complicated because I'm in the process of refactoring the entire crate, of which only roughly 20% is currently done.
I'll try to improve this in the future, hopefully once the rewrite is done this will be much improved. I did something similar with Mio, but I did the refactoring offline and only contributed the changes back once it was complete. Perhaps it was a mistake to take a different approach to socket2. |
I'm sorry to hear that. If the refactor is not finished though, why can't you review this and give me time to implement feedback before continuing with the refactor? There is no need to rush anything, including the review. It's just the new PRs that you're opening that make it impossible for me to contribute. Especially if you're pressed for time, you can let me do the legwork.. |
I created pr #125 just as an example, it's not complete, it doesn't even compile. I just wanted to show the direction I was thinking in, it's often easier to show with (pseude/broken) code that in English for me.
If you can help review #124 and rebase this pr on it with, with the suggested changes in line with #125 that would be great. |
Ah, that does sound much better. I'll make some time for that :) |
The socket types in the sys modules did not close their FD when dropped. Additionally, implementing FromRawFd/Socket was done by wrapping in a (now) owning handle and then calling `as_raw_fd()`, which would cause a double close.
Ok, rebased and processed the suggestions. The PR also hides the public Platform specific APIs needed by /edit: Also added tests to check the default flags (and their absence for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid this isn't quite what I was looking for. I'm added a bunch of inline comments.
Also please don't use the Self
key when returning or creating type, it requires someone to needlessly look up what type is being created, just use the concrete type name.
Creating sockets without the recommended flags can be done with new *_raw functions.
You could just leave this bit off. The in-line comments are quite clear on their own, and that kind of remark makes me feel somewhat unappreciated. Anyway, I think I processed most of the comments. I marked the processed ones as resolved, and we probably need to discuss the remaining ones a bit more. |
@de-vri-es I've rebased this on my branch in #134. |
I'm sorry, but I'm done here. I can't contribute to a repository where the maintainer does things like this. You may have rebased my PR, but you also modified it. Now it's up to me to search for modifications and comment if I don't agree with the changes. Finally, I would hugely appreciate it if you use a more human friendly tone in your feedback. I'd much prefer "Could you X", "I'd prefer Y" over "Revert X", "Do Y". I'm not a robot after all. I wish you the best of luck with this repo, but I can't contribute to it like this. |
Really... we've been through this. I don't have all the time in the world you review your pr.
It's very common to accept a PR with changes. I did it in the same commit, but I also could have made it another commit, really who cares?
I've been doing too long to say please, especially since a thank you is rarely around the corner. I think you're taking these things way to personal. I've always kept my comments about your code and never made it personal, I can't say the same about your comments (which made it tiring you work you to be honest, but I never mentioned that did I?).
Thank you. Even though we might not see eye to eye now I also wish you the best of luck. |
This is my proposal for #111: the default behavior is to enable close-on-exec and
SOCK_NOSIGPIPE
on the relevant platforms, but there are*_raw
functions that really only wrap a single syscall.