From 4b0778590349e2ff2b07073615f1b82ad83d35de Mon Sep 17 00:00:00 2001 From: Ruediger Pluem Date: Wed, 3 May 2023 07:47:15 +0000 Subject: 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 --- memcache/apr_memcache.c | 75 ++++++++++++-- test/Makefile.in | 5 + test/Makefile.win | 6 ++ test/NWGNUmakefile | 1 + test/NWGNUmemcachedmock | 252 ++++++++++++++++++++++++++++++++++++++++++++++++ test/memcachedmock.c | 84 ++++++++++++++++ test/testmemcache.c | 69 +++++++++++++ test/testmemcache.h | 24 +++++ 8 files changed, 509 insertions(+), 7 deletions(-) create mode 100644 test/NWGNUmemcachedmock create mode 100644 test/memcachedmock.c create mode 100644 test/testmemcache.h 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 +#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 /* 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 + -- cgit v1.2.1