ZOOKEEPER-4210: Preserve return code from nonblocking send

[smikes]

Async API calls attempt to flush the send buffer, which
calls flush_send_queue(); and can report
ZOPERATIONTIMEOUT
ZSYSTEMERROR
ZCONNECTIONLOSS

Specifically: send_buffer() calls send(2) with MSG_NOSIGNAL,
which can return EPIPE; then send_buffer return -1, causing
ZCONNECTIONLOSS from flush_send_queue().

Current async API calls drop the return value from flush_send_queue(),
as below:

adaptor_send_queue(zh, 0);
return (rc < 0)?ZMARSHALLINGERROR:ZOK;

The async API then returns ZOK instead of ZCONNECTIONLOSS.

[smikes]

Additional notes:

• developed against 3.4 and adapted to 3.6
• allows earlier discovery of connection loss

[smikes]

Failure in PR-build:

[2021-02-12T23:45:10.488Z] [ERROR] Errors: 
[2021-02-12T23:45:10.488Z] [ERROR]   ReadOnlyModeTest.testConnectionEvents:205 » Timeout Failed to connect in read-...
[2021-02-12T23:45:10.488Z] [INFO] 
[2021-02-12T23:45:10.488Z] [ERROR] Tests run: 2901, Failures: 0, Errors: 1, Skipped: 4

I will try to reproduce the failure locally.

[maoling]

@smikes

• Thanks for this contribution. It's better to create a JIRA issue (sign up JIRA if you don't have an account)
  to bind this PR to a JIRA-ID. ZooKeeper uses the GitHub workflow. The contributor guideline is here
• The CI failure may be not your fault and you can ignore it:)

[smikes]

Thanks. I created https://issues.apache.org/jira/browse/ZOOKEEPER-4210

[maoling]

Ping @ztzg @eolivelli @anmolnar :)

[smikes]

Confirmed that ReadOnlyModeTest passes locally, so if this is known to be flaky/slow under load, then I'm fine.

[smikes]

Running the build on branch-3.6 I see some unit tests failing, both with this change and without

[ztzg]

Hi @smikes,

Looks good, in general. Thank you!

But…

This is technically an API & ABI break, as it is widening the range of error codes returned by API functions. As such, I would recommend not including it in upcoming 3.6 releases. (I would be okay with 3.7+ as 1/ C only uses integer codes, which most programs don't interpret and 2/ we are not on the "semantic versioning" train. @eolivelli, WDYT?)

In any case, we usually take PRs against master and backport to the relevant branches; would you mind doing that?

I have also left a couple of comments on the diff, one of which is actionable.

Finally, there is the question of documenting the change in zookeeper.h; the comment on e.g. zoo_aset is now outdated and should probably be rephrased:

/* [...]
 * \return ZOK on success or one of the following errcodes on failure:
 * ZBADARGUMENTS - invalid input parameters
 * ZINVALIDSTATE - zhandle state is either ZOO_SESSION_EXPIRED_STATE or ZOO_AUTH_FAILED_STATE
 * ZMARSHALLINGERROR - failed to marshall a request; possibly, out of memory
 */
ZOOAPI int zoo_aset(zhandle_t *zh, const char *path, const char *buffer, int buflen,
        int version, stat_completion_t completion, const void *data);

Would you agree?

Cheers, -D

P.-S. — You commented:

Running the build on branch-3.6 I see some unit tests failing, both with this change and without

Right; we have some stability issues with the current test suite. Passing -Dsurefire-forkcount=1 helps with Java tests, at the cost of a looong runtime. Something we have to fix ASAP, but please ignore CI failures which don't seem relevant in the meantime, as @maoling suggested.

[smikes]

Sounds good, will take a look. Internally we have 3.4 and 3.6 branches active so I developed against that, I will rebase this against the main branch as you suggest

[smikes]

Summary of current changes:

• bc5c997
  A revised version of the original change. This now DOES NOT change the ABI -- it does not modify the return values that come from the zoo_a* functions.
  Also, the action on ZCONNECTIONLOSS is more conservative. Now, a connection loss that is detected during an opportunistic nonblocking send will be treated as a normal connection loss, not an error. The pending send buffers will not be cleared, and completions/watchers will be preserved at least until the next zookeeper_interest call.

• 9c0cd16
  Addresses the comment above, that handle_socket_error_msg reports the caller's line but not the caller's function name. Not needed to address ZOOKEEPER-4210, so I would be happy to split it into a different PR. Please advise.

• e20bf70
  In addition to changing the behavior when connection loss is detected, also allow the return code ZCONNECTIONLOSS to propagate upward to the zoo_a* functions. Add documentation in zookeeper.h for the new return codes.
  Note: included for completeness, but I now think this is probably a bad idea, because there is nothing the consumer of the zookeeper.c API can do with the new information (about connection loss during async send).

[smikes]

Separately: I was seeing the zktest_mt and zktest_st cppunit test suites fail, because of my change to the ABI, so I am hoping this will now build cleanly.

[ztzg]

@smikes: Still planning to have a look at this, and #1609, ASAP. Thank you for all that work so far!

[ztzg]

LGTM!

@eolivelli, @anmolnar, @symat: PTAL.

[ztzg]

Summary of current changes:

• bc5c997
  A revised version of the original change. This now DOES NOT change the ABI -- it does not modify the return values that come from the zoo_a* functions.
  Also, the action on ZCONNECTIONLOSS is more conservative. Now, a connection loss that is detected during an opportunistic nonblocking send will be treated as a normal connection loss, not an error. The pending send buffers will not be cleared, and completions/watchers will be preserved at least until the next zookeeper_interest call.

This is this PR. LGTM.

• 9c0cd16
  Addresses the comment above, that handle_socket_error_msg reports the caller's line but not the caller's function name. Not needed to address ZOOKEEPER-4210, so I would be happy to split it into a different PR. Please advise.

Now #1609 and ZOOKEEPER-4217. Thank you for splitting it.

I note that the PR currently is in "draft" state. Do you have specific improvements in mind?

• e20bf70
  In addition to changing the behavior when connection loss is detected, also allow the return code ZCONNECTIONLOSS to propagate upward to the zoo_a* functions. Add documentation in zookeeper.h for the new return codes.
  Note: included for completeness, but I now think this is probably a bad idea, because there is nothing the consumer of the zookeeper.c API can do with the new information (about connection loss during async send).

Agree. (But thank you for preparing the commit anyway!)

[smikes]

Summary of current changes:

• bc5c997
  A revised version of the original change. This now DOES NOT change the ABI -- it does not modify the return values that come from the zoo_a* functions.

This is this PR. LGTM.

Cool! I am happy to take more feedback on this, or it can be merged.

• 9c0cd16
  Addresses the comment above, that handle_socket_error_msg reports the caller's line but not the caller's function name. Not needed to address ZOOKEEPER-4210, so I would be happy to split it into a different PR. Please advise.

Now #1609 and ZOOKEEPER-4217. Thank you for splitting it.

I note that the PR currently is in "draft" state. Do you have specific improvements in mind?

No. Today I removed the "draft" status.

After this, I will make some documentation updates to the C client dev notes.

[eolivelli]

Overall looks good to me,
but I left one question

[ztzg]

LGTM; thanks!

[ztzg]

This is now in master. Thank you for the contribution, and for your patience, @smikes!