Check sockets from connection pool before using them and try to reconnect

them if they are not usable any longer.

* memcache/apr_memcache.c::ms_find_conn:
    Check if the socket returned from the connection pool is still readable. If
    not then invalidate the connection in the pool and request a new one
    from the connection pool. Repeat this until a valid socket is returned
    or this was done the maximum number of connections in the pool plus one.
    This ensures that at least one new socket was created. If a new socket
    does not work this indicates a broken backend and not just a restart in
    the past. In this case return an error like previously.

* test/testmemcache.c:
    Add new test for connection validation.

* test/memcachedmock.c:
    For the new test we need a memcached mock server that we control and
    can restart.

* test/testmemcache.h:
    Shared defines between test/testmemcache.c and test/memcachedmock.c.

* test/Makefile.in:
* test/Makefile.win:
* test/NWGNUmakefile:
* test/NWGNUmemcachedmock:
    Needed changes to build test/memcachedmock.c on different platforms.


git-svn-id: https://svn.apache.org/repos/asf/apr/apr/trunk@1909585 13f79535-47bb-0310-9956-ffa450edef68

8 files changed, 509 insertions, 7 deletions

diff --git a/memcache/apr_memcache.c b/memcache/apr_memcache.c
index 52e29e19e..5c028dbef 100644
--- a/memcache/apr_memcache.c
+++ b/memcache/apr_memcache.c
@@ -222,22 +222,83 @@ APR_DECLARE(apr_memcache_server_t *) apr_memcache_find_server(apr_memcache_t *mc
     return NULL;
 }
 
+
+/* Forward declare mc_conn_construct */
+static apr_status_t
+mc_conn_construct(void **conn_, void *params, apr_pool_t *pool);
+
 static apr_status_t ms_find_conn(apr_memcache_server_t *ms, apr_memcache_conn_t **conn)
 {
-    apr_status_t rv;
+    apr_status_t rv = APR_SUCCESS;
     apr_bucket_alloc_t *balloc;
     apr_bucket *e;
+#if APR_HAS_THREADS
+    int i;
+#endif
+    int atreadeof;
 
 #if APR_HAS_THREADS
-    rv = apr_reslist_acquire(ms->conns, (void **)conn);
+    /*
+     * In order to avoid an endless loop in case that a freshly connected
+     * socket is immediately closed by the remote side we limit this loop
+     * to the maxium number of connections in the reslist plus 1.
+     */
+    for (i = 0; i <= ms->max; i++) {
+        rv = apr_reslist_acquire(ms->conns, (void **)conn);
+
+        if (rv != APR_SUCCESS) {
+            return rv;
+        }
+
+        atreadeof = 0;
+        rv = apr_socket_atreadeof((*conn)->sock, &atreadeof);
+
+        if ((rv == APR_SUCCESS) && !atreadeof) {
+            break;
+        }
+        /*
+         * The socket we got is fishy. But maybe the memcached was just
+         * restarted. Hence give it a chance by destroying the socket and
+         * getting a new one.
+         */
+        rv = apr_reslist_invalidate(ms->conns, *conn);
+        *conn = NULL;
+
+        if (rv != APR_SUCCESS) {
+            return rv;
+        }
+    }
+
+    /*
+     * Even after refreshing all sockets we still do not have a working one.
+     * Give up.
+     */
+    if (!(*conn)) {
+        return APR_EGENERAL;
+    }
 #else
     *conn = ms->conn;
-    rv = APR_SUCCESS;
-#endif
-
-    if (rv != APR_SUCCESS) {
-        return rv;
+    atreadeof = 0;
+    if (*conn) {
+        rv = apr_socket_atreadeof((*conn)->sock, &atreadeof);
+        if ((rv != APR_SUCCESS) || !atreadeof) {
+            ms->conn = NULL;
+            apr_pool_destroy((*conn)->p);
+            rv = mc_conn_construct((void**)conn, ms, ms->p);
+            if (rv != APR_SUCCESS) {
+                return rv;
+            }
+            ms->conn = *conn;
+        }
+    }
+    else {
+        rv = mc_conn_construct((void**)conn, ms, ms->p);
+        if (rv != APR_SUCCESS) {
+            return rv;
+        }
+        ms->conn = *conn;
     }
+#endif
 
     balloc = apr_bucket_alloc_create((*conn)->tp);
     (*conn)->bb = apr_brigade_create((*conn)->tp, balloc);
diff --git a/test/Makefile.in b/test/Makefile.in
index d32fb3182..4af81ab60 100644
--- a/test/Makefile.in
+++ b/test/Makefile.in
@@ -52,6 +52,7 @@ TESTALL_COMPONENTS = \
 	proc_child@EXEEXT@ \
 	readchild@EXEEXT@ \
 	sockchild@EXEEXT@ \
+	memcachedmock@EXEEXT@ \
 	testshmproducer@EXEEXT@ \
 	testshmconsumer@EXEEXT@ \
 	tryread@EXEEXT@ \
@@ -156,6 +157,10 @@ OBJECTS_sockchild = sockchild.lo $(LOCAL_LIBS)
 sockchild@EXEEXT@: $(OBJECTS_sockchild)
 	$(LINK_PROG) $(OBJECTS_sockchild) $(ALL_LIBS)
 
+OBJECTS_memcachedmock = memcachedmock.lo $(LOCAL_LIBS)
+memcachedmock@EXEEXT@: $(OBJECTS_memcachedmock)
+	$(LINK_PROG) $(OBJECTS_memcachedmock) $(ALL_LIBS)
+
 OBJECTS_testshmconsumer = testshmconsumer.lo $(LOCAL_LIBS)
 testshmconsumer@EXEEXT@: $(OBJECTS_testshmconsumer) $(LOCAL_LIBS)
 	$(LINK_PROG) $(OBJECTS_testshmconsumer) $(ALL_LIBS)
diff --git a/test/Makefile.win b/test/Makefile.win
index 21344ea03..ff96bee33 100644
--- a/test/Makefile.win
+++ b/test/Makefile.win
@@ -73,6 +73,7 @@ TESTALL_COMPONENTS = \
 	$(OUTDIR)\proc_child.exe \
         $(OUTDIR)\tryread.exe \
 	$(OUTDIR)\sockchild.exe \
+	$(OUTDIR)\memcachedmock.exe \
 	$(OUTDIR)\testshmproducer.exe \
 	$(OUTDIR)\testshmconsumer.exe \
 	$(OUTDIR)\globalmutexchild.exe
@@ -274,6 +275,11 @@ $(OUTDIR)\sockchild.exe: $(INTDIR)\sockchild.obj $(LOCAL_LIB)
 	@if exist "$@.manifest" \
 	    mt.exe -manifest "$@.manifest" -outputresource:$@;1
 
+$(OUTDIR)\memcachedmock.exe: $(INTDIR)\memcachedmock.obj $(LOCAL_LIB)
+	$(LD) $(LDFLAGS) /out:"$@" $** $(LD_LIBS)
+	@if exist "$@.manifest" \
+	    mt.exe -manifest "$@.manifest" -outputresource:$@;1
+
 $(OUTDIR)\testshmconsumer.exe: $(INTDIR)\testshmconsumer.obj $(LOCAL_LIB)
 	$(LD) $(LDFLAGS) /out:"$@" $** $(LD_LIBS)
 	@if exist "$@.manifest" \
diff --git a/test/NWGNUmakefile b/test/NWGNUmakefile
index 350e6652e..a26d2c80c 100644
--- a/test/NWGNUmakefile
+++ b/test/NWGNUmakefile
@@ -169,6 +169,7 @@ TARGET_nlm = \
 	$(OBJDIR)/proc_child.nlm \
 	$(OBJDIR)/readchild.nlm \
 	$(OBJDIR)/sockchild.nlm \
+	$(OBJDIR)/memcachedmock.nlm \
 	$(OBJDIR)/sockperf.nlm \
 	$(OBJDIR)/testatmc.nlm \
 	$(OBJDIR)/tryread.nlm \
diff --git a/test/NWGNUmemcachedmock b/test/NWGNUmemcachedmock
new file mode 100644
index 000000000..db35f34b1
--- /dev/null
+++ b/test/NWGNUmemcachedmock
@@ -0,0 +1,252 @@
+#
+# Make sure all needed macro's are defined
+#
+
+#
+# Get the 'head' of the build environment if necessary.  This includes default
+# targets and paths to tools
+#
+
+ifndef EnvironmentDefined
+include $(APR_WORK)/build/NWGNUhead.inc
+endif
+
+#
+# These directories will be at the beginning of the include list, followed by
+# INCDIRS
+#
+XINCDIRS	+= \
+			$(APR)/include \
+			$(APR)/include/arch/netware \
+			$(EOLIST)
+
+#
+# These flags will come after CFLAGS
+#
+XCFLAGS		+= \
+			$(EOLIST)
+
+#
+# These defines will come after DEFINES
+#
+XDEFINES	+= \
+			$(EOLIST)
+
+#
+# These flags will be added to the link.opt file
+#
+XLFLAGS		+= \
+			$(EOLIST)
+
+#
+# These values will be appended to the correct variables based on the value of
+# RELEASE
+#
+ifeq "$(RELEASE)" "debug"
+XINCDIRS	+= \
+			$(EOLIST)
+
+XCFLAGS		+= \
+			$(EOLIST)
+
+XDEFINES	+= \
+			$(EOLIST)
+
+XLFLAGS		+= \
+			$(EOLIST)
+endif
+
+ifeq "$(RELEASE)" "noopt"
+XINCDIRS	+= \
+			$(EOLIST)
+
+XCFLAGS		+= \
+			$(EOLIST)
+
+XDEFINES	+= \
+			$(EOLIST)
+
+XLFLAGS		+= \
+			$(EOLIST)
+endif
+
+ifeq "$(RELEASE)" "release"
+XINCDIRS	+= \
+			$(EOLIST)
+
+XCFLAGS		+= \
+			$(EOLIST)
+
+XDEFINES	+= \
+			$(EOLIST)
+
+XLFLAGS		+= \
+			$(EOLIST)
+endif
+
+#
+# These are used by the link target if an NLM is being generated
+# This is used by the link 'name' directive to name the nlm.  If left blank
+# TARGET_nlm (see below) will be used.
+#
+NLM_NAME	= memcachedmock
+
+#
+# This is used by the link '-desc ' directive. 
+# If left blank, NLM_NAME will be used.
+#
+NLM_DESCRIPTION	= socket NLM to test sockets
+
+#
+# This is used by the '-threadname' directive.  If left blank,
+# NLM_NAME Thread will be used.
+#
+NLM_THREAD_NAME	= $(NLM_NAME)
+
+#
+# This is used by the '-screenname' directive.  If left blank,
+# 'Apache for NetWare' Thread will be used.
+#
+NLM_SCREEN_NAME = DEFAULT
+
+#
+# If this is specified, it will override VERSION value in 
+# $(APR_WORK)/build/NWGNUenvironment.inc
+#
+NLM_VERSION	=
+
+#
+# If this is specified, it will override the default of 64K
+#
+NLM_STACK_SIZE	= 
+
+#
+# If this is specified it will be used by the link '-entry' directive
+#
+NLM_ENTRY_SYM	=
+
+#
+# If this is specified it will be used by the link '-exit' directive
+#
+NLM_EXIT_SYM	=
+
+#
+# If this is specified it will be used by the link '-check' directive
+#
+NLM_CHECK_SYM	=
+
+#
+# If this is specified it will be used by the link '-flags' directive
+#
+NLM_FLAGS	= AUTOUNLOAD, PSEUDOPREEMPTION, MULTIPLE
+ 
+#
+# If this is specified it will be linked in with the XDCData option in the def 
+# file instead of the default of $(APR)/misc/netware/apache.xdc.  XDCData can 
+# be disabled by setting APACHE_UNIPROC in the environment
+#
+XDCDATA		= 
+
+#
+# Declare all target files (you must add your files here)
+#
+
+#
+# If there is an NLM target, put it here
+#
+TARGET_nlm = \
+	$(OBJDIR)/$(NLM_NAME).nlm \
+	$(EOLIST)
+
+#
+# If there is an LIB target, put it here
+#
+TARGET_lib = \
+	$(EOLIST)
+
+#
+# These are the OBJ files needed to create the NLM target above.
+# Paths must all use the '/' character
+#
+FILES_nlm_objs = \
+	$(OBJDIR)/$(NLM_NAME).o \
+	$(EOLIST)
+
+#
+# These are the LIB files needed to create the NLM target above.
+# These will be added as a library command in the link.opt file.
+#
+FILES_nlm_libs = \
+	$(PRELUDE) \
+	$(EOLIST)
+
+#
+# These are the modules that the above NLM target depends on to load.
+# These will be added as a module command in the link.opt file.
+#
+FILES_nlm_modules = \
+	aprlib \
+	libc \
+	$(EOLIST)
+
+#
+# If the nlm has a msg file, put it's path here
+#
+FILE_nlm_msg =
+ 
+#
+# If the nlm has a hlp file put it's path here
+#
+FILE_nlm_hlp =
+
+#
+# If this is specified, it will override the default copyright.
+#
+FILE_nlm_copyright =
+
+#
+# Any additional imports go here
+#
+FILES_nlm_Ximports = \
+	@$(APR)/aprlib.imp \
+	@$(NOVI)/libc.imp \
+	$(EOLIST)
+ 
+#   
+# Any symbols exported to here
+#
+FILES_nlm_exports = \
+	$(EOLIST)
+
+#   
+# These are the OBJ files needed to create the LIB target above.
+# Paths must all use the '/' character
+#
+FILES_lib_objs = \
+	$(EOLIST)
+
+#
+# implement targets and dependancies (leave this section alone)
+#
+
+libs :: $(OBJDIR) $(TARGET_lib)
+
+nlms :: libs $(TARGET_nlm)
+
+#
+# Updated this target to create necessary directories and copy files to the 
+# correct place.  (See $(APR_WORK)/build/NWGNUhead.inc for examples)
+#
+install :: nlms FORCE
+
+#
+# Any specialized rules here
+#
+
+#
+# Include the 'tail' makefile that has targets that depend on variables defined
+# in this makefile
+#
+
+include $(APRBUILD)/NWGNUtail.inc
+
diff --git a/test/memcachedmock.c b/test/memcachedmock.c
new file mode 100644
index 000000000..b6c2e6fce
--- /dev/null
+++ b/test/memcachedmock.c
@@ -0,0 +1,84 @@
+/* Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <stdlib.h>
+#include "apr_network_io.h"
+#include "apr_pools.h"
+#include "apr_pools.h"
+#include "testmemcache.h"
+
+#define MOCK_REPLY "VERSION 1.5.22\r\n"
+
+int main(int argc, char *argv[])
+{
+    apr_pool_t *p;
+    apr_sockaddr_t *sa;
+    apr_socket_t *server;
+    apr_socket_t *server_connection;
+    apr_status_t rv;
+    apr_size_t length;
+    int i;
+
+    apr_initialize();
+    atexit(apr_terminate);
+    apr_pool_create(&p, NULL);
+
+    apr_sockaddr_info_get(&sa, MOCK_HOST, APR_UNSPEC, MOCK_PORT, 0, p);
+
+    apr_socket_create(&server, sa->family, SOCK_STREAM, 0, p);
+
+    apr_socket_opt_set(server, APR_SO_REUSEADDR, 1);
+
+    apr_socket_timeout_set(server, 0);
+
+    apr_socket_bind(server, sa);
+
+    apr_socket_listen(server, 5);
+
+    /* Do spin instead of a proper poll for sake of simplicity */
+    for (i = 0; i < 4; i++) {
+
+        rv = apr_socket_accept(&server_connection, server, p);
+        if (rv == APR_SUCCESS) {
+            break;
+        }
+
+        apr_sleep(apr_time_from_sec(1));
+    }
+
+    length = strlen(MOCK_REPLY);
+    apr_socket_send(server_connection, MOCK_REPLY, &length);
+
+    apr_socket_close(server_connection);
+
+    /* Do spin instead of a proper poll for sake of simplicity */
+    for (i = 0; i < 4; i++) {
+
+        rv = apr_socket_accept(&server_connection, server, p);
+        if (rv == APR_SUCCESS) {
+            break;
+        }
+
+        apr_sleep(apr_time_from_sec(1));
+    }
+
+    length = strlen(MOCK_REPLY);
+    apr_socket_send(server_connection, MOCK_REPLY, &length);
+
+    apr_socket_close(server_connection);
+
+    exit(0);
+}
diff --git a/test/testmemcache.c b/test/testmemcache.c
index 33e0e7c45..20fad58d1 100644
--- a/test/testmemcache.c
+++ b/test/testmemcache.c
@@ -23,6 +23,8 @@
 #include "apr_memcache.h"
 #include "apr_network_io.h"
 #include "apr_thread_proc.h"
+#include "apr_signal.h"
+#include "testmemcache.h"
 
 #if APR_HAVE_STDLIB_H
 #include <stdlib.h>             /* for exit() */
@@ -662,6 +664,72 @@ static void test_memcache_setget(abts_case * tc, void *data)
     }
 }
 
+static void test_connection_validation(abts_case *tc, void *data)
+{
+    apr_status_t rv;
+    apr_memcache_t *memcache;
+    apr_memcache_server_t *memserver;
+    char *result;
+    apr_procattr_t *procattr;
+    apr_proc_t proc;
+    const char *args[2];
+    int exitcode;
+    apr_exit_why_e why;
+#ifdef SIGPIPE
+    /*
+     * If SIGPIPE is present ignore it as we will write to a closed socket.
+     * Otherwise we would be terminated by the default handler for SIGPIPE.
+     */
+    apr_sigfunc_t *old_action;
+
+    old_action = apr_signal(SIGPIPE, SIG_IGN);
+#endif
+
+    rv = apr_procattr_create(&procattr, p);
+    ABTS_ASSERT(tc, "Couldn't create procattr", rv == APR_SUCCESS);
+
+    rv = apr_procattr_io_set(procattr, APR_NO_PIPE, APR_NO_PIPE,
+            APR_NO_PIPE);
+    ABTS_ASSERT(tc, "Couldn't set io in procattr", rv == APR_SUCCESS);
+
+    rv = apr_procattr_error_check_set(procattr, 1);
+    ABTS_ASSERT(tc, "Couldn't set error check in procattr", rv == APR_SUCCESS);
+
+    rv = apr_procattr_cmdtype_set(procattr, APR_PROGRAM_ENV);
+    ABTS_ASSERT(tc, "Couldn't set copy environment", rv == APR_SUCCESS);
+
+    args[0] = "memcachedmock" EXTENSION;
+    args[1] = NULL;
+    rv = apr_proc_create(&proc, TESTBINPATH "memcachedmock" EXTENSION, args, NULL,
+                         procattr, p);
+    ABTS_ASSERT(tc, "Couldn't launch program", rv == APR_SUCCESS);
+
+    /* Wait for the mock memcached to start */
+    apr_sleep(apr_time_from_sec(2));
+
+    rv = apr_memcache_create(p, 1, 0, &memcache);
+    ABTS_ASSERT(tc, "memcache create failed", rv == APR_SUCCESS);
+
+    rv = apr_memcache_server_create(p, MOCK_HOST, MOCK_PORT, 0, 1, 1, 60000, &memserver);
+    ABTS_ASSERT(tc, "server create failed", rv == APR_SUCCESS);
+
+    rv = apr_memcache_add_server(memcache, memserver);
+    ABTS_ASSERT(tc, "server add failed", rv == APR_SUCCESS);
+
+    rv = apr_memcache_version(memserver, p, &result);
+    ABTS_ASSERT(tc, "Couldn't get initial version", rv == APR_SUCCESS);
+
+    rv = apr_memcache_version(memserver, p, &result);
+    ABTS_ASSERT(tc, "Couldn't get version after connection shutdown", rv == APR_SUCCESS);
+
+#ifdef SIGPIPE
+    /* Restore old SIGPIPE handler */
+    apr_signal(SIGPIPE, old_action);
+#endif
+
+    apr_proc_wait(&proc, &exitcode, &why, APR_WAIT);
+}
+
 abts_suite *testmemcache(abts_suite * suite)
 {
     suite = ADD_SUITE(suite);
@@ -672,6 +740,7 @@ abts_suite *testmemcache(abts_suite * suite)
     abts_run_test(suite, test_memcache_multiget, NULL);
     abts_run_test(suite, test_memcache_addreplace, NULL);
     abts_run_test(suite, test_memcache_incrdecr, NULL);
+    abts_run_test(suite, test_connection_validation, NULL);
 
     return suite;
 }
diff --git a/test/testmemcache.h b/test/testmemcache.h
new file mode 100644
index 000000000..e7f7066e6
--- /dev/null
+++ b/test/testmemcache.h
@@ -0,0 +1,24 @@
+/* Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#ifndef TESTMEMCACHE_H
+#define TESTMEMCACHE_H
+
+#define MOCK_HOST "localhost"
+#define MOCK_PORT 11231
+
+#endif
+