[ZOOKEEPER-2338] - set SOCK_CLOEXEC on socket if defined

[fr0stbyte]

No description provided.

[afine]

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?

[fr0stbyte]

@afine Thanks for looking at the PR
Here is the bug tracked on Mesos side : https://issues.apache.org/jira/browse/MESOS-4065

[afine]

@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?

[anmolnar]

@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.

[phunt]

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?

[fr0stbyte]

@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.

[phunt]

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?

[fr0stbyte]

@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.

[fr0stbyte]

@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

[fr0stbyte]

@phunt @anmolnar any suggestion on changes or does this look good ?

[anmolnar]

@fr0stbyte According to my tiny C knowledge, it looks good to me.

Are u going to create a separate PR for trunk?

[phunt]

I tested it with cmake and autotools based generation and the command lines work properly with agreed upon defaults. LGTM. +1.

[phunt]

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 !