Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

item_size_max support #8

Closed
wants to merge 4 commits into from

3 participants

@bmatheny

This patch causes object sizes to be checked before forwarding the request to the appropriate server. If the object is larger than this value (defaults to 1048576), the proxy closes the connection (which is also what memcached does). The proxy does NOT respond with a SERVER_ERROR, mostly because I couldn't determine a reasonable way to do that from the req_filter.

In a number of client libraries, compression occurs within the library and so you don't know whether a value exceeds the maximum object size until after it leaves this client. This allows you to pass 'large' items into a client method but not send the request across the wire if it would fail.

@travisbot

This pull request passes (merged 9a17a01 into b85ac82).

@travisbot

This pull request passes (merged f34559a into b85ac82).

@bmatheny

One additional comment. Based on feedback from Manju I moved the old vlen values to vlen_rem, and made vlen be an immutable value representing the total size of the value. This allows a much cleaner calculation in the req_filter of whether the object value exceeds the configured item_size_max or not.

@manjuraj
Collaborator

blake the patch looks great. One thing I realized is that memcache has some item header overhead and slab header overhead even for the largest item. So, even if you configure memcache with 1MB slab, the largest item that can be stored in the slab is < 1MB. Furthermore the item size not only includes the value length, but also key length.

Given this, do you think we should have two extra keys in yml configuration
item_max_kvlen: (maximum key + value length)
item_overhead:

and we discard requests whose key + value length + overhead > item_max_kvlen

thoughts?

@bmatheny

I'm going to close this and reopen with the changes you recommended and also the merge conflicts handled.

@bmatheny bmatheny closed this
@idning idning referenced this pull request from a commit in idning/twemproxy
@idning idning nutcracker core on heavy multi-del
today we got a core of twemproxy::

    $ gdb -c core.14420 ./bin/nutcracker

    (gdb) bt
    #0  0x000000302af2e2ed in raise () from /lib64/tls/libc.so.6
    #1  0x000000302af2fa3e in abort () from /lib64/tls/libc.so.6
    #2  0x0000000000419c82 in nc_assert (cond=0x444dc0 "!TAILQ_EMPTY(&send_msgq) && nsend != 0", file=0x444aa8 "nc_message.c", line=745, panic=1) at nc_util.c:308
    #3  0x000000000040d0d6 in msg_send_chain (ctx=0x553090, conn=0x55b380, msg=0x0) at nc_message.c:745
    #4  0x000000000040d568 in msg_send (ctx=0x553090, conn=0x55b380) at nc_message.c:820
    #5  0x00000000004059af in core_send (ctx=0x553090, conn=0x55b380) at nc_core.c:173
    #6  0x0000000000405ffe in core_core (arg=0x55b380, events=65280) at nc_core.c:301
    #7  0x0000000000429297 in event_wait (evb=0x5652e0, timeout=389) at nc_epoll.c:269
    #8  0x000000000040606f in core_loop (ctx=0x553090) at nc_core.c:316
    #9  0x000000000041b109 in nc_run (nci=0x7fbfffea80) at nc.c:530
    #10 0x000000000041b20d in main (argc=14, argv=0x7fbfffecc8) at nc.c:579
    (gdb) f 3
    #3  0x000000000040d0d6 in msg_send_chain (ctx=0x553090, conn=0x55b380, msg=0x0) at nc_message.c:745
    745         ASSERT(!TAILQ_EMPTY(&send_msgq) && nsend != 0);
    (gdb) l
    740             if (msg == NULL) {
    741                 break;
    742             }
    743         }
    744
    745         ASSERT(!TAILQ_EMPTY(&send_msgq) && nsend != 0);
    746
    747         conn->smsg = NULL;
    748
    749         n = conn_sendv(conn, &sendv, nsend);

it is caused by this ``ASSERT`` at nc_message.c:745,

``conn_send`` send no more than ``NC_IOV_MAX(128)`` pieces in ``msg_send_chain``,

if the first fragment of MULTI-DEL response is send on last batch. and this is the last msg in send queue, the next call of ``msg_send_chain`` will got ``nsend == 0``::

following case show such a case:
1. mget on ``126`` keys
2. a mutli-del cmd

::

    def test_multi_delete_20140525():
        conn = redis.Redis('127.0.0.5', 4100)
        cnt = 126
        keys = ['key-%s'%i for i in range(cnt)]
        pipe = conn.pipeline(transaction=False)
        pipe.mget(keys)
        pipe.delete(*keys)
        print pipe.execute()

see: https://github.com/idning/test-twemproxy/blob/master/test_redis/test_del.py#L56-L63

more detail: http://idning.github.io/twemproxy-core-20140523.html
d5ec284
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 31, 2012
  1. @bmatheny
  2. @bmatheny
  3. @bmatheny

    Formatting

    bmatheny authored
Commits on Nov 20, 2012
  1. @bmatheny
This page is out of date. Refresh to see the latest.
View
1  README.md
@@ -73,6 +73,7 @@ nutcracker can be configured through a YAML file specified by the -c or --conf-f
+ **distribution**: The key distribution mode. Possible values are: ketama, modula and random.
+ **timeout**: The timeout value in msec that we wait for to establish a connection to the server or receive a response from a server. By default, we wait indefinitely.
+ **backlog**: The TCP backlog argument. Defaults to 512.
++ **item_size_max**: The maximum size of an object. Defaults to 1048576. If you have a custom memcached build with a larger size, it can be adjusted here.
+ **preconnect**: A boolean value that controls if nutcracker should preconnect to all the servers in this pool on process start. Defaults to false.
+ **server_connections**: The maximum number of connections that can be opened to each server. By default, we open at most 1 server connection.
+ **auto_eject_hosts**: A boolean value that controls if server should be ejected temporarily when it fails consecutively server_failure_limit times. Defaults to false.
View
82 autogen.sh
@@ -0,0 +1,82 @@
+#!/bin/sh
+
+function assert_command() {
+ TEST=$(which $1 2>/dev/null)
+ if [[ $? -eq 0 ]]; then
+ return;
+ else
+ echo "Command '$1' missing"
+ exit 1
+ fi
+}
+
+function cleanup() {
+ if [ -f Makefile ]; then
+ make distclean
+ fi
+
+ rm -f Makefile.in \
+ aclocal.m4 \
+ compile \
+ config.h.in \
+ configure \
+ depcomp \
+ install-sh \
+ missing \
+ src/Makefile.in \
+ *~ \
+ m4/*
+
+ rm -rf autom4te.cache
+}
+
+function autogen() {
+ assert_command 'autoreconf'
+ exec autoreconf -ivf
+
+ LIBTOOLIZE=libtoolize
+ SYSNAME=$(uname)
+ if [ "x$SYSNAME" = "xDarwin" ] ; then
+ LIBTOOLIZE=glibtoolize
+ fi
+
+ assert_command 'automake'
+ assert_command $LIBTOOLIZE
+ assert_command 'aclocal'
+ assert_command 'autoheader'
+ assert_command 'autoconf'
+
+ aclocal -I m4 && \
+ autoheader && \
+ $LIBTOOLIZE && \
+ autoconf && \
+ automake --add-missing --force-missing --copy
+}
+
+function usage() {
+cat << EOF
+usage: $0 [options]
+
+Create configure script using autotools, or cleanup
+
+OPTIONS:
+ -h Show this message
+ -c Cleanup
+EOF
+}
+
+while getopts "hc" OPTION
+do
+ case $OPTION in
+ c)
+ cleanup
+ exit
+ ;;
+ ?)
+ usage
+ exit
+ ;;
+ esac
+done
+
+autogen
View
11 src/nc_conf.c
@@ -61,6 +61,10 @@ static struct command conf_commands[] = {
conf_set_num,
offsetof(struct conf_pool, backlog) },
+ { string("item_size_max"),
+ conf_set_num,
+ offsetof(struct conf_pool, item_size_max) },
+
{ string("client_connections"),
conf_set_num,
offsetof(struct conf_pool, client_connections) },
@@ -169,6 +173,7 @@ conf_pool_init(struct conf_pool *cp, struct string *name)
cp->distribution = CONF_UNSET_DIST;
cp->timeout = CONF_UNSET_NUM;
cp->backlog = CONF_UNSET_NUM;
+ cp->item_size_max = CONF_UNSET_NUM;
cp->client_connections = CONF_UNSET_NUM;
@@ -256,6 +261,7 @@ conf_pool_each_transform(void *elem, void *data)
sp->key_hash = hash_algos[cp->hash];
sp->timeout = cp->timeout;
sp->backlog = cp->backlog;
+ sp->item_size_max = (uint32_t)cp->item_size_max;
sp->client_connections = (uint32_t)cp->client_connections;
@@ -300,6 +306,7 @@ conf_dump(struct conf *cf)
log_debug(LOG_VVERB, " hash: %d", cp->hash);
log_debug(LOG_VVERB, " timeout: %d", cp->timeout);
log_debug(LOG_VVERB, " backlog: %d", cp->backlog);
+ log_debug(LOG_VVERB, " item_size_max: %d", cp->item_size_max);
log_debug(LOG_VVERB, " distribution: %d", cp->distribution);
log_debug(LOG_VVERB, " client_connections: %d",
cp->client_connections);
@@ -1186,6 +1193,10 @@ conf_validate_pool(struct conf *cf, struct conf_pool *cp)
cp->backlog = CONF_DEFAULT_LISTEN_BACKLOG;
}
+ if (cp->item_size_max == CONF_UNSET_NUM) {
+ cp->item_size_max = CONF_DEFAULT_ITEM_SIZE_MAX;
+ }
+
cp->client_connections = CONF_DEFAULT_CLIENT_CONNECTIONS;
if (cp->preconnect == CONF_UNSET_NUM) {
View
2  src/nc_conf.h
@@ -45,6 +45,7 @@
#define CONF_DEFAULT_DIST DIST_KETAMA
#define CONF_DEFAULT_TIMEOUT -1
#define CONF_DEFAULT_LISTEN_BACKLOG 512
+#define CONF_DEFAULT_ITEM_SIZE_MAX 1048576 /* in bytes */
#define CONF_DEFAULT_CLIENT_CONNECTIONS 0
#define CONF_DEFAULT_PRECONNECT false
#define CONF_DEFAULT_AUTO_EJECT_HOSTS false
@@ -76,6 +77,7 @@ struct conf_pool {
dist_type_t distribution; /* distribution: */
int timeout; /* timeout: */
int backlog; /* backlog: */
+ int item_size_max; /* item_size_max: */
int client_connections; /* client_connections: */
int preconnect; /* preconnect: */
int auto_eject_hosts; /* auto_eject_hosts: */
View
1  src/nc_message.c
@@ -224,6 +224,7 @@ _msg_get(void)
msg->key_start = NULL;
msg->key_end = NULL;
msg->vlen = 0;
+ msg->vlen_rem = 0;
msg->end = NULL;
msg->frag_id = 0;
View
1  src/nc_message.h
@@ -75,6 +75,7 @@ struct msg {
uint8_t *key_start; /* key start */
uint8_t *key_end; /* key end */
uint32_t vlen; /* value length */
+ uint32_t vlen_rem; /* value length remaining for parse phase */
uint8_t *end; /* end marker */
uint64_t frag_id; /* id of fragmented message */
View
23 src/nc_parse.c
@@ -393,7 +393,7 @@ parse_request(struct msg *r)
}
/* vlen_start <- p */
r->token = p;
- r->vlen = (uint32_t)(ch - '0');
+ r->vlen_rem = (uint32_t)(ch - '0');
state = SW_VLEN;
}
@@ -401,7 +401,7 @@ parse_request(struct msg *r)
case SW_VLEN:
if (ch >= '0' && ch <= '9') {
- r->vlen = r->vlen * 10 + (uint32_t)(ch - '0');
+ r->vlen_rem = r->vlen_rem * 10 + (uint32_t)(ch - '0');
} else if (r->cas) {
if (ch != ' ') {
goto error;
@@ -409,11 +409,13 @@ parse_request(struct msg *r)
/* vlen_end <- p - 1 */
p = p - 1; /* go back by 1 byte */
r->token = NULL;
+ r->vlen = r->vlen_rem;
state = SW_SPACES_BEFORE_CAS;
} else if (ch == ' ' || ch == CR) {
/* vlen_end <- p - 1 */
p = p - 1; /* go back by 1 byte */
r->token = NULL;
+ r->vlen = r->vlen_rem;
state = SW_RUNTO_CRLF;
} else {
goto error;
@@ -463,10 +465,10 @@ parse_request(struct msg *r)
break;
case SW_VAL:
- m = p + r->vlen;
+ m = p + r->vlen_rem;
if (m >= b->last) {
- ASSERT(r->vlen >= (uint32_t)(b->last - p));
- r->vlen -= (uint32_t)(b->last - p);
+ ASSERT(r->vlen_rem >= (uint32_t)(b->last - p));
+ r->vlen_rem -= (uint32_t)(b->last - p);
m = b->last - 1;
p = m; /* move forward by vlen bytes */
break;
@@ -932,7 +934,7 @@ parse_response(struct msg *r)
}
/* vlen_start <- p */
r->token = p;
- r->vlen = (uint32_t)(ch - '0');
+ r->vlen_rem = (uint32_t)(ch - '0');
state = SW_VLEN;
}
@@ -940,11 +942,12 @@ parse_response(struct msg *r)
case SW_VLEN:
if (ch >= '0' && ch <= '9') {
- r->vlen = r->vlen * 10 + (uint32_t)(ch - '0');
+ r->vlen_rem = r->vlen_rem * 10 + (uint32_t)(ch - '0');
} else if (ch == ' ' || ch == CR) {
/* vlen_end <- p - 1 */
p = p - 1; /* go back by 1 byte */
r->token = NULL;
+ r->vlen = r->vlen_rem;
state = SW_RUNTO_CRLF;
} else {
goto error;
@@ -966,10 +969,10 @@ parse_response(struct msg *r)
break;
case SW_VAL:
- m = p + r->vlen;
+ m = p + r->vlen_rem;
if (m >= b->last) {
- ASSERT(r->vlen >= (uint32_t)(b->last - p));
- r->vlen -= (uint32_t)(b->last - p);
+ ASSERT(r->vlen_rem >= (uint32_t)(b->last - p));
+ r->vlen_rem -= (uint32_t)(b->last - p);
m = b->last - 1;
p = m; /* move forward by vlen bytes */
break;
View
15 src/nc_request.c
@@ -357,6 +357,8 @@ req_recv_next(struct context *ctx, struct conn *conn, bool alloc)
static bool
req_filter(struct context *ctx, struct conn *conn, struct msg *msg)
{
+ struct server_pool *pool = conn->owner;
+
ASSERT(conn->client && !conn->proxy);
if (msg_empty(msg)) {
@@ -381,6 +383,19 @@ req_filter(struct context *ctx, struct conn *conn, struct msg *msg)
return true;
}
+ /* Handle excessively large request payloads */
+ if (msg->vlen > pool->item_size_max) {
+ ASSERT(conn->rmsg == NULL);
+ log_debug(LOG_ERR, "filter size %"PRIu32" > max %"PRIu32" req %"PRIu64" from c %d",
+ msg->vlen, pool->item_size_max, msg->id, conn->sd);
+ conn->done = 1;
+ req_put(msg);
+ return true;
+ } else {
+ log_debug(LOG_DEBUG, "filter size %"PRIu32" req %"PRIu64" from c %d",
+ msg->vlen, msg->id, conn->sd);
+ }
+
return false;
}
View
1  src/nc_server.h
@@ -111,6 +111,7 @@ struct server_pool {
hash_t key_hash; /* key hasher */
int timeout; /* timeout in msec */
int backlog; /* listen backlog */
+ uint32_t item_size_max; /* maximum size object (bytes) */
uint32_t client_connections; /* maximum # client connection */
uint32_t server_connections; /* maximum # server connection */
int64_t server_retry_timeout; /* server retry timeout in usec */
Something went wrong with that request. Please try again.