Skip to content
Browse files

MB-3479 - Use binary protocol EBUSY & EINTERNAL instead of ENOMEM

Instead of over-using the OOM / ENOMEM binary protocol response
error...

- Return EBUSY during a timeout.
- Return EINTERNAL for closed sockets & down servers.
- Return ENOMEM when memcached returns ENOMEM.

Also, use EINTERNAL rather than ENOMEM as the generic catch-all error
code, which should reduce confusion ("but, I'm not actually out of
memory").

Change-Id: I207903f0c4d5b967866c67cb61ac3b43a832d5cd
Reviewed-on: http://review.membase.org/6177
Tested-by: Steve Yen <steve.yen@gmail.com>
Reviewed-by: Steve Yen <steve.yen@gmail.com>
  • Loading branch information...
1 parent cb5fbbf commit 5fa3a33486c4a51fe2367fd3555decb28cf143d2 @steveyen steveyen committed
Showing with 74 additions and 27 deletions.
  1. +38 −24 cproxy.c
  2. +6 −2 cproxy.h
  3. +1 −0 cproxy_multiget.c
  4. +2 −0 cproxy_protocol_a.c
  5. +2 −0 cproxy_protocol_a2a.c
  6. +6 −1 cproxy_protocol_a2b.c
  7. +1 −0 cproxy_protocol_b2b.c
  8. +18 −0 memcached.c
View
62 cproxy.c
@@ -37,8 +37,8 @@ conn *conn_list_remove(conn *head, conn **tail,
bool is_compatible_request(conn *existing, conn *candidate);
-void propagate_error(downstream *d);
-void propagate_error_msg(downstream *d, char *ascii_msg);
+void propagate_error_msg(downstream *d, char *ascii_msg,
+ protocol_binary_response_status binary_status);
void downstream_reserved_time_sample(proxy_stats_td *ptds, uint64_t duration);
void downstream_connect_time_sample(proxy_stats_td *ptds, uint64_t duration);
@@ -569,6 +569,7 @@ void cproxy_on_close_upstream_conn(conn *c) {
if (d->upstream_conn == NULL) {
d->upstream_suffix = NULL;
d->upstream_suffix_len = 0;
+ d->upstream_status = PROTOCOL_BINARY_RESPONSE_SUCCESS;
d->upstream_retry = 0;
// Don't need to do anything else, as we'll now just
@@ -732,6 +733,8 @@ void cproxy_on_close_downstream_conn(conn *c) {
if (IS_ASCII(d->upstream_conn->protocol)) {
d->upstream_suffix = "SERVER_ERROR proxy downstream closed\r\n";
d->upstream_suffix_len = 0;
+ } else {
+ d->upstream_status = PROTOCOL_BINARY_RESPONSE_EINTERNAL;
}
d->upstream_retry = 0;
@@ -765,6 +768,7 @@ void cproxy_on_close_downstream_conn(conn *c) {
uc_retry = d->upstream_conn;
d->upstream_suffix = NULL;
d->upstream_suffix_len = 0;
+ d->upstream_status = PROTOCOL_BINARY_RESPONSE_SUCCESS;
d->upstream_retry = 0;
}
}
@@ -775,9 +779,9 @@ void cproxy_on_close_downstream_conn(conn *c) {
IS_BINARY(d->upstream_conn->protocol)) {
protocol_binary_response_header *rh =
cproxy_make_bin_error(d->upstream_conn,
- PROTOCOL_BINARY_RESPONSE_ENOMEM);
+ PROTOCOL_BINARY_RESPONSE_EINTERNAL);
if (rh != NULL) {
- d->upstream_suffix = (char *)rh;
+ d->upstream_suffix = (char *) rh;
d->upstream_suffix_len = sizeof(protocol_binary_response_header);
} else {
d->ptd->stats.stats.err_oom++;
@@ -892,6 +896,7 @@ downstream *cproxy_reserve_downstream(proxy_td *ptd) {
assert(d->upstream_conn == NULL);
assert(d->upstream_suffix == NULL);
assert(d->upstream_suffix_len == 0);
+ assert(d->upstream_status == PROTOCOL_BINARY_RESPONSE_SUCCESS);
assert(d->upstream_retry == 0);
assert(d->downstream_used == 0);
assert(d->downstream_used_start == 0);
@@ -903,6 +908,7 @@ downstream *cproxy_reserve_downstream(proxy_td *ptd) {
d->upstream_conn = NULL;
d->upstream_suffix = NULL;
d->upstream_suffix_len = 0;
+ d->upstream_status = PROTOCOL_BINARY_RESPONSE_SUCCESS;
d->upstream_retry = 0;
d->upstream_retries = 0;
d->usec_start = 0;
@@ -994,7 +1000,7 @@ bool cproxy_release_downstream(downstream *d, bool force) {
} else {
d->ptd->stats.stats.tot_downstream_propagate_failed++;
- propagate_error(d);
+ propagate_error_msg(d, NULL, d->upstream_status);
}
} else {
if (settings.verbose > 2) {
@@ -1050,10 +1056,11 @@ bool cproxy_release_downstream(downstream *d, bool force) {
}
if (settings.verbose > 2) {
- moxi_log_write("%d: release_downstream upstream_suffix %s\n",
+ moxi_log_write("%d: release_downstream upstream_suffix %s status %x\n",
d->upstream_conn->sfd,
d->upstream_suffix_len == 0 ?
- d->upstream_suffix : "(binary)");
+ d->upstream_suffix : "(binary)",
+ d->upstream_status);
}
if (d->upstream_suffix != NULL) {
@@ -1117,6 +1124,7 @@ bool cproxy_release_downstream(downstream *d, bool force) {
d->upstream_conn = NULL;
d->upstream_suffix = NULL; // No free(), expecting a static string.
d->upstream_suffix_len = 0;
+ d->upstream_status = PROTOCOL_BINARY_RESPONSE_SUCCESS;
d->upstream_retry = 0;
d->upstream_retries = 0;
d->usec_start = 0;
@@ -1750,7 +1758,9 @@ void cproxy_assign_downstream(proxy_td *ptd) {
}
uc->next = NULL;
- upstream_error(uc);
+ upstream_error_msg(uc,
+ "SERVER_ERROR proxy out of downstreams\r\n",
+ PROTOCOL_BINARY_RESPONSE_EINTERNAL);
}
}
@@ -1826,7 +1836,7 @@ void cproxy_assign_downstream(proxy_td *ptd) {
break;
}
- propagate_error(d);
+ propagate_error_msg(d, NULL, d->upstream_status);
cproxy_release_downstream(d, false);
}
@@ -1837,11 +1847,8 @@ void cproxy_assign_downstream(proxy_td *ptd) {
}
}
-void propagate_error(downstream *d) {
- propagate_error_msg(d, NULL);
-}
-
-void propagate_error_msg(downstream *d, char *ascii_msg) {
+void propagate_error_msg(downstream *d, char *ascii_msg,
+ protocol_binary_response_status binary_status) {
assert(d != NULL);
while (d->upstream_conn != NULL) {
@@ -1852,7 +1859,7 @@ void propagate_error_msg(downstream *d, char *ascii_msg) {
uc->sfd);
}
- upstream_error_msg(uc, ascii_msg);
+ upstream_error_msg(uc, ascii_msg, binary_status);
conn *curr = d->upstream_conn;
d->upstream_conn = d->upstream_conn->next;
@@ -1899,7 +1906,7 @@ bool cproxy_forward(downstream *d) {
bool cproxy_forward_or_error(downstream *d) {
if (cproxy_forward(d) == false) {
d->ptd->stats.stats.tot_downstream_propagate_failed++;
- propagate_error(d);
+ propagate_error_msg(d, NULL, d->upstream_status);
cproxy_release_downstream(d, false);
return false;
@@ -1908,11 +1915,8 @@ bool cproxy_forward_or_error(downstream *d) {
return true;
}
-void upstream_error(conn *uc) {
- upstream_error_msg(uc, NULL);
-}
-
-void upstream_error_msg(conn *uc, char *ascii_msg) {
+void upstream_error_msg(conn *uc, char *ascii_msg,
+ protocol_binary_response_status binary_status) {
assert(uc);
assert(uc->state == conn_pause);
@@ -1958,7 +1962,13 @@ void upstream_error_msg(conn *uc, char *ascii_msg) {
} else {
assert(IS_BINARY(uc->protocol));
- write_bin_error(uc, PROTOCOL_BINARY_RESPONSE_ENOMEM, 0);
+ if (binary_status == PROTOCOL_BINARY_RESPONSE_SUCCESS) {
+ // Default to our favorite catch-all binary protocol response.
+ //
+ binary_status = PROTOCOL_BINARY_RESPONSE_EINTERNAL;
+ }
+
+ write_bin_error(uc, binary_status, 0);
update_event(uc, EV_WRITE | EV_PERSIST);
}
@@ -2015,6 +2025,7 @@ bool cproxy_dettach_if_noreply(downstream *d, conn *uc) {
d->upstream_conn = NULL;
d->upstream_suffix = NULL;
d->upstream_suffix_len = 0;
+ d->upstream_status = PROTOCOL_BINARY_RESPONSE_SUCCESS;
d->upstream_retry = 0;
cproxy_reset_upstream(uc);
@@ -2232,7 +2243,9 @@ void wait_queue_timeout(const int fd,
&ptd->waiting_any_downstream_tail,
uc, NULL); // TODO: O(N^2).
- upstream_error(uc);
+ upstream_error_msg(uc,
+ "SERVER_ERROR proxy wait queue timeout",
+ PROTOCOL_BINARY_RESPONSE_EBUSY);
}
}
@@ -2610,7 +2623,8 @@ void downstream_timeout(const int fd,
ptd->stats.stats.tot_downstream_timeout++;
}
- propagate_error_msg(d, "SERVER_ERROR proxy downstream timeout\r\n");
+ propagate_error_msg(d, "SERVER_ERROR proxy downstream timeout\r\n",
+ PROTOCOL_BINARY_RESPONSE_EBUSY);
int n = mcs_server_count(&d->mst);
View
8 cproxy.h
@@ -465,6 +465,10 @@ struct downstream {
conn *upstream_conn; // Non-NULL when downstream is reserved.
char *upstream_suffix; // Last bit to write when downstreams are done.
int upstream_suffix_len; // When >0, overrides strlen(upstream_suffix) for binary.
+
+ // Used during an error when upstream is binary protocol.
+ protocol_binary_response_status upstream_status;
+
int upstream_retry; // Will be >0 if we should retry the entire
// command again when all downstreams are done.
// Used in not-my-vbucket error case. During
@@ -561,8 +565,8 @@ bool cproxy_update_event_write(downstream *d, conn *c);
bool cproxy_forward(downstream *d);
-void upstream_error(conn *uc);
-void upstream_error_msg(conn *uc, char *ascii_msg);
+void upstream_error_msg(conn *uc, char *ascii_msg,
+ protocol_binary_response_status binary_status);
void upstream_retry(void *data0, void *data1);
int downstream_conn_index(downstream *d, conn *c);
View
1 cproxy_multiget.c
@@ -359,6 +359,7 @@ bool multiget_ascii_downstream(downstream *d, conn *uc,
if (cproxy_dettach_if_noreply(d, uc) == false) {
d->upstream_suffix = "END\r\n";
d->upstream_suffix_len = 0;
+ d->upstream_status = PROTOCOL_BINARY_RESPONSE_SUCCESS;
d->upstream_retry = 0;
cproxy_start_downstream_timeout(d, NULL);
View
2 cproxy_protocol_a.c
@@ -488,6 +488,7 @@ bool cproxy_optimize_set_ascii(downstream *d, conn *uc,
d->upstream_conn = NULL;
d->upstream_suffix = NULL;
d->upstream_suffix_len = 0;
+ d->upstream_status = PROTOCOL_BINARY_RESPONSE_SUCCESS;
d->upstream_retry = 0;
out_string(uc, "STORED");
@@ -579,6 +580,7 @@ void cproxy_ascii_broadcast_suffix(downstream *d) {
}
d->upstream_suffix_len = 0;
+ d->upstream_status = PROTOCOL_BINARY_RESPONSE_SUCCESS;
d->upstream_retry = 0;
}
}
View
2 cproxy_protocol_a2a.c
@@ -159,6 +159,7 @@ void cproxy_process_a2a_downstream(conn *c, char *line) {
} else if (strncmp(line, "LOCK_ERROR", 10) == 0) {
d->upstream_suffix = "LOCK_ERROR\r\n";
d->upstream_suffix_len = 0;
+ d->upstream_status = PROTOCOL_BINARY_RESPONSE_ETMPFAIL;
conn_set_state(c, conn_pause);
} else {
conn_set_state(c, conn_pause);
@@ -487,6 +488,7 @@ bool cproxy_broadcast_a2a_downstream(downstream *d,
if (cproxy_dettach_if_noreply(d, uc) == false) {
d->upstream_suffix = suffix;
d->upstream_suffix_len = 0;
+ d->upstream_status = PROTOCOL_BINARY_RESPONSE_SUCCESS;
d->upstream_retry = 0;
cproxy_start_downstream_timeout(d, NULL);
View
7 cproxy_protocol_a2b.c
@@ -753,8 +753,9 @@ void a2b_process_downstream_response(conn *c) {
* currently membase does not send ETMPFAIL for
* engine error code for ENGINE_TMPFAIL
*/
- d->upstream_suffix_len = 0;
d->upstream_suffix = "LOCK_ERROR\r\n";
+ d->upstream_suffix_len = 0;
+ d->upstream_status = status;
}
conn_set_state(c, conn_pause);
@@ -1104,6 +1105,7 @@ bool cproxy_forward_a2b_simple_downstream(downstream *d,
uc->cmd_curr == PROTOCOL_BINARY_CMD_GETL) {
d->upstream_suffix = "END\r\n";
d->upstream_suffix_len = 0;
+ d->upstream_status = PROTOCOL_BINARY_RESPONSE_SUCCESS;
d->upstream_retry = 0;
}
@@ -1304,6 +1306,7 @@ bool cproxy_forward_a2b_simple_downstream(downstream *d,
if (d->upstream_suffix == NULL) {
d->upstream_suffix = "SERVER_ERROR a2b event oom\r\n";
d->upstream_suffix_len = 0;
+ d->upstream_status = PROTOCOL_BINARY_RESPONSE_ENOMEM;
d->upstream_retry = 0;
}
}
@@ -1318,6 +1321,7 @@ bool cproxy_forward_a2b_simple_downstream(downstream *d,
if (d->upstream_suffix == NULL) {
d->upstream_suffix = "CLIENT_ERROR a2b parse request\r\n";
d->upstream_suffix_len = 0;
+ d->upstream_status = PROTOCOL_BINARY_RESPONSE_EINVAL;
d->upstream_retry = 0;
}
}
@@ -1479,6 +1483,7 @@ bool cproxy_broadcast_a2b_downstream(downstream *d,
if (cproxy_dettach_if_noreply(d, uc) == false) {
d->upstream_suffix = suffix;
d->upstream_suffix_len = 0;
+ d->upstream_status = PROTOCOL_BINARY_RESPONSE_SUCCESS;
d->upstream_retry = 0;
cproxy_start_downstream_timeout(d, NULL);
View
1 cproxy_protocol_b2b.c
@@ -291,6 +291,7 @@ bool cproxy_broadcast_b2b_downstream(downstream *d, conn *uc) {
if (add_conn_item(uc, it)) {
d->upstream_suffix = ITEM_data(it);
d->upstream_suffix_len = it->nbytes;
+ d->upstream_status = PROTOCOL_BINARY_RESPONSE_SUCCESS;
if (settings.verbose > 2) {
moxi_log_write("%d: b2b broadcast upstream_suffix", uc->sfd);
View
18 memcached.c
@@ -992,6 +992,24 @@ void write_bin_error(conn *c, protocol_binary_response_status err, int swallow)
case PROTOCOL_BINARY_RESPONSE_AUTH_ERROR:
errstr = "Auth failure";
break;
+ case PROTOCOL_BINARY_RESPONSE_AUTH_CONTINUE:
+ errstr = "Auth continue";
+ break;
+ case PROTOCOL_BINARY_RESPONSE_NOT_MY_VBUCKET:
+ errstr = "Not my vbucket";
+ break;
+ case PROTOCOL_BINARY_RESPONSE_NOT_SUPPORTED:
+ errstr = "Not supported";
+ break;
+ case PROTOCOL_BINARY_RESPONSE_EINTERNAL:
+ errstr = "Internal error";
+ break;
+ case PROTOCOL_BINARY_RESPONSE_EBUSY:
+ errstr = "System is busy";
+ break;
+ case PROTOCOL_BINARY_RESPONSE_ETMPFAIL:
+ errstr = "Temporary failure";
+ break;
default:
assert(false);
errstr = "UNHANDLED ERROR";

0 comments on commit 5fa3a33

Please sign in to comment.
Something went wrong with that request. Please try again.