You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2021/02/17 20:20:37 UTC
[GitHub] [zookeeper] smikes opened a new pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
smikes opened a new pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes commented on pull request #1602: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes commented on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-778516841
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes edited a comment on pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes edited a comment on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-780952654
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).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes edited a comment on pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes edited a comment on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-780954496
Separately: I am seeing the `zktest_mt` and `zktest_st` cppunit test suites fail, it seems spuriously, on some log management calls. I might be able to fix this by using mocks for the log tests... also a new PR/JIRA issue?
``` [exec] Zookeeper_simpleSystem::testLogCallbackClearLog Message Received: [2021-02-17 17:42:23,777:207110(0x7f13ca702740):ZOO_INFO@log_env@1250: Client environment:zookeeper.version=zookeeper C client 3.8.0]
....
[exec] Log Message Received: [2021-02-17 17:42:24,611:207110(0x7f13c9e8f700):ZOO_ERROR@check_events@2983: Socket 127.0.0.1:22181 zk retcode=-4, errno=111(Connection refused): server refused to accept the client]
[exec] Log Message Received: [2021-02-17 17:42:24,611:207110(0x7f13c9e8f700):ZOO_WARN@zookeeper_interest@2561: Delaying connection after exhaustively trying all servers [127.0.0.1:22181]]
[exec] : elapsed 1001 : OK
[exec] terminate called after throwing an instance of 'CppUnit::Exception'
[exec] what(): equality assertion failed
[exec] - Expected: -101
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes commented on pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes commented on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-780952654
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).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes commented on pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes commented on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-780108485
Running the build on branch-3.6 I see some unit tests failing, both with this change and without
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes commented on pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes commented on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-780025114
Confirmed that `ReadOnlyModeTest` passes locally, so if this is known to be flaky/slow under load, then I'm fine.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#discussion_r577453743
##########
File path: zookeeper-client/zookeeper-client-c/src/zookeeper.c
##########
@@ -3730,8 +3745,9 @@ int zoo_awget(zhandle_t *zh, const char *path,
LOG_DEBUG(LOGCALLBACK(zh), "Sending request xid=%#x for path [%s] to %s",h.xid,path,
zoo_get_current_server(zh));
+
/* make a best (non-blocking) effort to send the requests asap */
- adaptor_send_queue(zh, 0);
+ return nonblocking_send(zh, rc);
return (rc < 0)?ZMARSHALLINGERROR:ZOK;
Review comment:
Leftover `return` statement.
##########
File path: zookeeper-client/zookeeper-client-c/src/zookeeper.c
##########
@@ -3691,6 +3691,21 @@ static int Request_path_watch_init(zhandle_t *zh, int mode,
/*---------------------------------------------------------------------------*
* ASYNC API
*---------------------------------------------------------------------------*/
+#define nonblocking_send(zh, rc) _nonblocking_send(zh, rc, __LINE__)
+static int _nonblocking_send(zhandle_t *zh, int rc, int line)
+{
+ if (rc < 0) {
+ rc = ZMARSHALLINGERROR;
+ } else {
+ rc = adaptor_send_queue(zh, 0);
+ if (rc < 0) {
+ rc = handle_socket_error_msg(zh, line, rc, "Socket error during nonblocking send");
Review comment:
Passing an accurate `line` is nice, but I now realize that `handle_socket_error_msg` generates log lines such as `ZOO_ERROR@handle_socket_error_msg@1234` (I.e., correct line, wrong function).
(Just a remark, not asking you to fix it in this PR. Noted for later.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes closed pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes closed pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes commented on a change in pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes commented on a change in pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#discussion_r578041352
##########
File path: zookeeper-client/zookeeper-client-c/src/zookeeper.c
##########
@@ -3730,8 +3745,9 @@ int zoo_awget(zhandle_t *zh, const char *path,
LOG_DEBUG(LOGCALLBACK(zh), "Sending request xid=%#x for path [%s] to %s",h.xid,path,
zoo_get_current_server(zh));
+
/* make a best (non-blocking) effort to send the requests asap */
- adaptor_send_queue(zh, 0);
+ return nonblocking_send(zh, rc);
return (rc < 0)?ZMARSHALLINGERROR:ZOK;
Review comment:
Fixed, see new commits.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#discussion_r582692030
##########
File path: zookeeper-client/zookeeper-client-c/src/zookeeper.c
##########
@@ -3961,6 +3961,19 @@ static int Request_path_watch_init(zhandle_t *zh, int mode,
/*---------------------------------------------------------------------------*
* ASYNC API
*---------------------------------------------------------------------------*/
+
+/* make an attempt to send queued requests immediately without blocking */
+static int nonblocking_send(zhandle_t *zh, int rc)
+{
+ if (adaptor_send_queue(zh, 0) < 0) {
+ if (is_connected(zh)) {
+ close_zsock(zh->fd);
+ zh->state = ZOO_NOTCONNECTED_STATE;
+ }
+ }
+ return (rc < 0) ? ZMARSHALLINGERROR : ZOK;
Review comment:
@eolivelli wrote:
> in the previous version of this code we were basically always calling `adaptor_send_queue`, now in case of previous error (rc < 0) we are not calling that function anymore.
>
> can this have a behavioural impact ?
Good point, indeed. I don't think we have a way to be sure, so I prefer this version.
Now… sorry to be annoying, but:
I'm not sure I like the `is_connected(zh)` condition. You previously had `zh->fd->sock != -1` here, which seems to make more sense—as the client might *not* be connected when this function is called. (That can happen, e.g., if the client is in the middle of a `prime_connection` or a SASL authentication sequence; applications do not always wait for a state notification before they start issuing requests.)
WDYT?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes commented on a change in pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes commented on a change in pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#discussion_r580729927
##########
File path: zookeeper-client/zookeeper-client-c/src/zookeeper.c
##########
@@ -3961,6 +3961,23 @@ static int Request_path_watch_init(zhandle_t *zh, int mode,
/*---------------------------------------------------------------------------*
* ASYNC API
*---------------------------------------------------------------------------*/
+#define nonblocking_send(zh, rc) _nonblocking_send(zh, rc, __LINE__)
+static int _nonblocking_send(zhandle_t *zh, int rc, int line)
Review comment:
Thanks for pointing this out. Formerly this was used because the function called handle_socket_error_msg() on error. Since this is no longer called, we can omit the macro and the argument. Updated PR , force-pushed amended version f0539f292
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-779578202
Ping @ztzg @eolivelli @anmolnar :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes commented on pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes commented on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-780774963
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes commented on a change in pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes commented on a change in pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#discussion_r583979202
##########
File path: zookeeper-client/zookeeper-client-c/src/zookeeper.c
##########
@@ -3961,6 +3961,19 @@ static int Request_path_watch_init(zhandle_t *zh, int mode,
/*---------------------------------------------------------------------------*
* ASYNC API
*---------------------------------------------------------------------------*/
+
+/* make an attempt to send queued requests immediately without blocking */
+static int nonblocking_send(zhandle_t *zh, int rc)
+{
+ if (adaptor_send_queue(zh, 0) < 0) {
+ if (is_connected(zh)) {
+ close_zsock(zh->fd);
+ zh->state = ZOO_NOTCONNECTED_STATE;
+ }
+ }
+ return (rc < 0) ? ZMARSHALLINGERROR : ZOK;
Review comment:
Sounds reasonable. Will return to using `zh->fd != -1` as the guard for calling `close_zsock()` ; new version 037a4f884 is pushed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes commented on pull request #1602: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes commented on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-779271425
Thanks. I created https://issues.apache.org/jira/browse/ZOOKEEPER-4210
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes commented on a change in pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes commented on a change in pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#discussion_r581493588
##########
File path: zookeeper-client/zookeeper-client-c/src/zookeeper.c
##########
@@ -3961,6 +3961,22 @@ static int Request_path_watch_init(zhandle_t *zh, int mode,
/*---------------------------------------------------------------------------*
* ASYNC API
*---------------------------------------------------------------------------*/
+static int nonblocking_send(zhandle_t *zh, int rc)
+{
+ if (rc < 0) {
+ rc = ZMARSHALLINGERROR;
+ } else {
+ rc = adaptor_send_queue(zh, 0);
Review comment:
Thanks, this is a good question. I have reorganized the code to minimize the change; now adaptor_send_queue() will always be called.
If an error is returned from adaptor_send_queue(), check if we are in a connected state (`is_connected`) and only then, close the socket and set state to `ZOO_NOTCONNECTED_STATE`.
Also: removed the replicated comment and collapsed the ZMARSHALLINGERROR check in `aremove_watches`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#discussion_r582692030
##########
File path: zookeeper-client/zookeeper-client-c/src/zookeeper.c
##########
@@ -3961,6 +3961,19 @@ static int Request_path_watch_init(zhandle_t *zh, int mode,
/*---------------------------------------------------------------------------*
* ASYNC API
*---------------------------------------------------------------------------*/
+
+/* make an attempt to send queued requests immediately without blocking */
+static int nonblocking_send(zhandle_t *zh, int rc)
+{
+ if (adaptor_send_queue(zh, 0) < 0) {
+ if (is_connected(zh)) {
+ close_zsock(zh->fd);
+ zh->state = ZOO_NOTCONNECTED_STATE;
+ }
+ }
+ return (rc < 0) ? ZMARSHALLINGERROR : ZOK;
Review comment:
@eolivelli wrote:
> in the previous version of this code we were basically always calling `adaptor_send_queue`, now in case of previous error (rc < 0) we are not calling that function anymore.
>
> can this have a behavioural impact ?
Good point, indeed. I don't think we have a way to be sure, so I prefer this version.
Now… sorry to be annoying, but:
I'm not sure I like the `is_connected(zh)` condition. You previously had `zh->fd->sock != -1` here, which seems to make more sense—as the client might *not* be connected when this function is called. (That can happen, e.g., if the client is in the middle of a `prime_connection` or a SASL authentication sequence. Aplications do not always wait for a connection notification before they start issuing requests.)
WDYT?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#discussion_r582692030
##########
File path: zookeeper-client/zookeeper-client-c/src/zookeeper.c
##########
@@ -3961,6 +3961,19 @@ static int Request_path_watch_init(zhandle_t *zh, int mode,
/*---------------------------------------------------------------------------*
* ASYNC API
*---------------------------------------------------------------------------*/
+
+/* make an attempt to send queued requests immediately without blocking */
+static int nonblocking_send(zhandle_t *zh, int rc)
+{
+ if (adaptor_send_queue(zh, 0) < 0) {
+ if (is_connected(zh)) {
+ close_zsock(zh->fd);
+ zh->state = ZOO_NOTCONNECTED_STATE;
+ }
+ }
+ return (rc < 0) ? ZMARSHALLINGERROR : ZOK;
Review comment:
@eolivelli wrote:
> in the previous version of this code we were basically always calling `adaptor_send_queue`, now in case of previous error (rc < 0) we are not calling that function anymore.
>
> can this have a behavioural impact ?
Good point, indeed. I don't think we have a way to be sure, so I prefer this version.
Now… sorry to be annoying, but:
I'm not sure I like the `is_connected(zh)` condition. You previously had `zh->fd->sock != -1` here, which seems to make more sense—as the client might *not* be connected when this function is called. (That can happen, e.g., if the client is in the middle of a `prime_connection` or a SASL authentication sequence. Aplications do not always wait for a connection notification before they start issuing requests.)
WDYT?
(This would also align the test & logic with `destroy` and `update_addrs`.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#discussion_r580073782
##########
File path: zookeeper-client/zookeeper-client-c/src/zookeeper.c
##########
@@ -3961,6 +3961,23 @@ static int Request_path_watch_init(zhandle_t *zh, int mode,
/*---------------------------------------------------------------------------*
* ASYNC API
*---------------------------------------------------------------------------*/
+#define nonblocking_send(zh, rc) _nonblocking_send(zh, rc, __LINE__)
+static int _nonblocking_send(zhandle_t *zh, int rc, int line)
Review comment:
this parameter `line ` is not used, why should we declare it ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg closed pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
ztzg closed pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] maoling commented on pull request #1602: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
maoling commented on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-779186147
@smikes
- Thanks for this contribution. It's better to create a [JIRA](https://issues.apache.org/jira/projects/ZOOKEEPER/) 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](https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToContribute)
- The CI failure may be not your fault and you can ignore it:)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes commented on a change in pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes commented on a change in pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#discussion_r583979202
##########
File path: zookeeper-client/zookeeper-client-c/src/zookeeper.c
##########
@@ -3961,6 +3961,19 @@ static int Request_path_watch_init(zhandle_t *zh, int mode,
/*---------------------------------------------------------------------------*
* ASYNC API
*---------------------------------------------------------------------------*/
+
+/* make an attempt to send queued requests immediately without blocking */
+static int nonblocking_send(zhandle_t *zh, int rc)
+{
+ if (adaptor_send_queue(zh, 0) < 0) {
+ if (is_connected(zh)) {
+ close_zsock(zh->fd);
+ zh->state = ZOO_NOTCONNECTED_STATE;
+ }
+ }
+ return (rc < 0) ? ZMARSHALLINGERROR : ZOK;
Review comment:
Sounds reasonable. Will return to using `zh->fd != -1` as the guard for calling `close_zsock()` ; new version bc8adb0b4 is pushed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes edited a comment on pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes edited a comment on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-780954496
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-782339189
@smikes: Still planning to have a look at this, and https://github.com/apache/zookeeper/pull/1609, ASAP. Thank you for all that work so far!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-782914541
> Summary of current changes:
>
> * [bc5c997](https://github.com/apache/zookeeper/commit/bc5c9975c08aa98748319ff1cd5c60b5602cba7d)
> 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](https://github.com/apache/zookeeper/commit/9c0cd1652a41269ce54120bb6656fcdd219063f7)
> 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 https://github.com/apache/zookeeper/pull/1609 and [ZOOKEEPER-4217](https://issues.apache.org/jira/browse/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](https://github.com/apache/zookeeper/commit/e20bf7015ed6f28d41d488e478ec308f627fb8a7)
> 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!)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes commented on pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes commented on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-780954496
Separately: I am seeing the `zktest_mt` and `zktest_st` cppunit test suites fail, it seems spuriously, on some log management calls:
``` [exec] Zookeeper_simpleSystem::testLogCallbackClearLog Message Received: [2021-02-17 17:42:23,777:207110(0x7f13ca702740):ZOO_INFO@log_env@1250: Client environment:zookeeper.version=zookeeper C client 3.8.0]
[exec] Log Message Received: [2021-02-17 17:42:24,611:207110(0x7f13c9e8f700):ZOO_ERROR@check_events@2983: Socket 127.0.0.1:22181 zk retcode=-4, errno=111(Connection refused): server refused to accept the client]
[exec] Log Message Received: [2021-02-17 17:42:24,611:207110(0x7f13c9e8f700):ZOO_WARN@zookeeper_interest@2561: Delaying connection after exhaustively trying all servers [127.0.0.1:22181]]
[exec] : elapsed 1001 : OK
[exec] terminate called after throwing an instance of 'CppUnit::Exception'
[exec] what(): equality assertion failed
[exec] - Expected: -101
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes commented on pull request #1602: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes commented on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-778509829
Additional notes:
- developed against 3.4 and adapted to 3.6
- allows earlier discovery of connection loss
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes edited a comment on pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes edited a comment on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-780954496
Separately: I am seeing the `zktest_mt` and `zktest_st` cppunit test suites fail, it seems spuriously, on some async functions -- not actually log management, I see now I might be able to fix this... also a new PR/JIRA issue?
```
[exec] Zookeeper_simpleSystem::testAsyncWatcherAutoResetFAIL: zktest-mt
[exec] terminate called after throwing an instance of 'CppUnit::Exception'
[exec] what(): equality assertion failed
[exec] - Expected: -101
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-794416188
This is now in `master`. Thank you for the contribution, and for your patience, @smikes!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes commented on a change in pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes commented on a change in pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#discussion_r578041689
##########
File path: zookeeper-client/zookeeper-client-c/src/zookeeper.c
##########
@@ -3691,6 +3691,21 @@ static int Request_path_watch_init(zhandle_t *zh, int mode,
/*---------------------------------------------------------------------------*
* ASYNC API
*---------------------------------------------------------------------------*/
+#define nonblocking_send(zh, rc) _nonblocking_send(zh, rc, __LINE__)
+static int _nonblocking_send(zhandle_t *zh, int rc, int line)
+{
+ if (rc < 0) {
+ rc = ZMARSHALLINGERROR;
+ } else {
+ rc = adaptor_send_queue(zh, 0);
+ if (rc < 0) {
+ rc = handle_socket_error_msg(zh, line, rc, "Socket error during nonblocking send");
Review comment:
I added a commit that addresses this, but it is incidental to the problem of connection loss, so it may be better to put that in a different PR. Please advise.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#discussion_r581140205
##########
File path: zookeeper-client/zookeeper-client-c/src/zookeeper.c
##########
@@ -3961,6 +3961,22 @@ static int Request_path_watch_init(zhandle_t *zh, int mode,
/*---------------------------------------------------------------------------*
* ASYNC API
*---------------------------------------------------------------------------*/
+static int nonblocking_send(zhandle_t *zh, int rc)
+{
+ if (rc < 0) {
+ rc = ZMARSHALLINGERROR;
+ } else {
+ rc = adaptor_send_queue(zh, 0);
Review comment:
(sorry I have one last question, I could have noticed it before).
in the previous version of this code we were basically always calling `adaptor_send_queue`, now in case of previous error (rc < 0) we are not calling that function anymore.
can this have a behavioural impact ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] smikes commented on pull request #1602: ZOOKEEPER-4210: Preserve return code from nonblocking send
Posted by GitBox <gi...@apache.org>.
smikes commented on pull request #1602:
URL: https://github.com/apache/zookeeper/pull/1602#issuecomment-782940825
> > Summary of current changes:
> >
> > * [bc5c997](https://github.com/apache/zookeeper/commit/bc5c9975c08aa98748319ff1cd5c60b5602cba7d)
> > 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](https://github.com/apache/zookeeper/commit/9c0cd1652a41269ce54120bb6656fcdd219063f7)
> > 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](https://issues.apache.org/jira/browse/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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org