You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@zookeeper.apache.org by fr0stbyte <gi...@git.apache.org> on 2017/10/27 15:04:05 UTC
[GitHub] zookeeper pull request #410: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket i...
GitHub user fr0stbyte opened a pull request:
https://github.com/apache/zookeeper/pull/410
[ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defined
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/fr0stbyte/zookeeper ZOOKEEPER-2338
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/zookeeper/pull/410.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #410
----
commit e7fb9fc9fb0140c35869af255d39e643597b517c
Author: Radu Brumariu <ra...@groupon.com>
Date: 2017-10-25T18:24:17Z
[ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defined
----
---
[GitHub] zookeeper issue #410: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/410
I tested it with cmake and autotools based generation and the command lines work properly with agreed upon defaults. LGTM. +1.
---
[GitHub] zookeeper issue #410: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/410
@fr0stbyte Thanks for linking to the Mesos bug. Unfortunately, I am not familiar with the way Mesos handles processes and I think it is possible that other members of the community share my ignorance.
Would you mind describing the conditions that are necessary to reproduce this bug?
---
[GitHub] zookeeper issue #410: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/410
@fr0stbyte According to my tiny C knowledge, it looks good to me.
---
[GitHub] zookeeper issue #410: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by fr0stbyte <gi...@git.apache.org>.
Github user fr0stbyte commented on the issue:
https://github.com/apache/zookeeper/pull/410
@phunt Let me know if the changes are inline with what you've had in mind. Once we clear that, I will add the changes for 3.6
---
[GitHub] zookeeper issue #410: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by fr0stbyte <gi...@git.apache.org>.
Github user fr0stbyte commented on the issue:
https://github.com/apache/zookeeper/pull/410
@phunt @anmolnar any suggestion on changes or does this look good ?
---
[GitHub] zookeeper pull request #410: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket i...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/zookeeper/pull/410
---
[GitHub] zookeeper issue #410: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/410
> I am not sure how
Yea, same here I'm afraid.
In thinking about it a bit perhaps what we could do is one of the following two option (might be more):
1) add it with some extra tooling in autoconf (and probably now cmake since it was recently introduced). Basically: add a new flag that the user (person compiling the library) could use to enable/disable SOCK_CLOEXEC when running ./configure.
In 3.5 the default behavior would be to not use SOCK_CLOEXEC regardless whether it's supported or not (user can override default with the option). In 3.6 and later we'd make SOCK_CLOEXEC the default and call it out as a potential issue as part of the release notes (disable with the option in this case).
2) it seems like we could do similar with a runtime property that the user could set in code prior to running zookeeper_init that would be global in nature. e.g. zoo_deterministic_conn_order
Again the defaults would be as specified in 1) above.
Thoughts?
---
[GitHub] zookeeper issue #410: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by fr0stbyte <gi...@git.apache.org>.
Github user fr0stbyte commented on the issue:
https://github.com/apache/zookeeper/pull/410
@phunt I'd have no problem making in backward compatible, but I am not sure how. Can you offer some guidance here ?
@anmolnar In the description of MESOS-4065 you can see 2 processes that share the same open file descriptor. In Linux ( Unix in general ) upon forking, the child inherits all the open files of the parent, unless the O_CLOEXEC option has been set on the file descriptor. My change uses the SOCK_CLOEXEC which is an atomic set of that flag. The alternative is to open it and then use `fcntl` to set the flag, but that leaves space for a race as a process could fork before.
Since the child process does not need / makes use of the open file descriptor it is proper to close it before exec'ing. I wouldn't say it's a functional bug, but rather good housekeeping ( afaict ).
@afine I think in order to reproduce it, you need a process that uses the C client, then that forks and execs something else. To verify, look at the open file descriptors for the 2 processes and verify that the open socket to zookeeper is not available in the child.
---
[GitHub] zookeeper issue #410: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/410
This looks like a non-backward compatible change to me. That said I'm not sure how many clients would rely on this behavior. thoughts?
---
[GitHub] zookeeper issue #410: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by fr0stbyte <gi...@git.apache.org>.
Github user fr0stbyte commented on the issue:
https://github.com/apache/zookeeper/pull/410
@afine Thanks for looking at the PR
Here is the bug tracked on Mesos side : https://issues.apache.org/jira/browse/MESOS-4065
---
[GitHub] zookeeper issue #410: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by anmolnar <gi...@git.apache.org>.
Github user anmolnar commented on the issue:
https://github.com/apache/zookeeper/pull/410
@fr0stbyte Reading some Stackoverflow about the flag (https://stackoverflow.com/questions/22304631/what-is-the-purpose-to-set-sock-cloexec-flag-with-accept4-same-as-o-cloexec), the change looks reasonable to me. However it would be beneficial to shed some light on how it's related to your scenario.
---
[GitHub] zookeeper issue #410: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by afine <gi...@git.apache.org>.
Github user afine commented on the issue:
https://github.com/apache/zookeeper/pull/410
Hi @fr0stbyte
I see in the JIRA that this ticked arises from an issue related to running ZooKeeper Mesos. Would you be able to explain exactly why this is needed?
---
[GitHub] zookeeper issue #410: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by phunt <gi...@git.apache.org>.
Github user phunt commented on the issue:
https://github.com/apache/zookeeper/pull/410
Committed to branch-3.5 and master. I tried applying to branch-3.4 but it wouldn't apply cleanly. Please submit another pull request for 3.4 and I'll review/commit that as well. Thanks @fr0stbyte !
---
[GitHub] zookeeper issue #410: [ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defin...
Posted by fr0stbyte <gi...@git.apache.org>.
Github user fr0stbyte commented on the issue:
https://github.com/apache/zookeeper/pull/410
@phunt both options seem fine to me. I think would prefer the first one, that way if the library remains ABI compatible, the clients won't need to recompile.
---