Alan DeKok [[email protected]] wrote:
> Sent: Friday, March 09, 2012 3:25 AM
> Brian Julin wrote:
> > This keeps the server listening, but there are some lingering issues:
>
> Well, fixes are welcome.
>
> I don't have time to look into this for a few weeks at least.
request_proxy_anew was assuming its argument would be installed in the
proxy_list, which wasn't the case, so it was removing it twice causing
.num_outgoing counters to roll over. Then, request_proxy was not expecting
the case where the argument was already in the proxy_list (put there by
request_proxy_anew) and was failing when attempting to add it a second
time. The latter makes me wonder why or if request_proxy_anew works at all.
The attached patch seems to do the trick. Some caveats:
This bypasses (for certain situations) the attempts to make sure that
a duplicate packet does not reuse the proxy_list ID of its predecessor.
Not knowing the reasoning behind that, I don't know if that's important
or not.
request_proxy has a "retransmit" flag as a parameter, which might be the
better test to avoid inserting the entry twice, or might not be.
Off topic, JOOC, while reading through the source I was left wondering what
prevents proxy_wait_for_reply from entering master-only functions from a
non-master thread when it falls through the DUP case into the TIMER case.
diff --git a/src/main/process.c b/src/main/process.c
index 4b5f084..f3b0c3f 100644
--- a/src/main/process.c
+++ b/src/main/process.c
@@ -1596,7 +1596,7 @@ static void remove_from_proxy_hash_nl(REQUEST *request)
request->proxy_listener = NULL;
/*
- * Got from YES in hash, to NO, not in hash while we hold
+ * Go from YES in hash, to NO, not in hash while we hold
* the mutex. This guarantees that when another thread
* grabs the mutex, the "not in hash" flag is correct.
*/
@@ -2264,7 +2264,7 @@ static int request_proxy(REQUEST *request, int retransmit)
/*
* We're actually sending a proxied packet. Do that now.
*/
- if (!insert_into_proxy_hash(request)) {
+ if (!request->in_proxy_hash && !insert_into_proxy_hash(request)) {
radlog_request(L_PROXY, 0, request, "Failed to insert initial packet into the proxy list.");
return -1;
}
@@ -2298,9 +2298,13 @@ static int request_proxy_anew(REQUEST *request)
/*
* Keep a copy of the old Id so that the
* re-transmitted request doesn't re-use the old
- * Id.
+ * Id. Note that in certain cases (socket crash)
+ * there is no Id as they have been purged from
+ * proxy_list, but there should still be a leftover
+ * packet hung off this request.
*/
RADIUS_PACKET old = *request->proxy;
+ int old_hash = request->in_proxy_hash;
home_server *home;
home_server *old_home = request->home_server;
#ifdef WITH_TCP
@@ -2327,7 +2331,7 @@ static int request_proxy_anew(REQUEST *request)
}
/*
- * Don't free the old Id on error.
+ * Don't free the old Id (if any) on error.
*/
if (!insert_into_proxy_hash(request)) {
radlog_request(L_PROXY, 0, request, "Failed to insert retransmission into the proxy list.");
@@ -2335,16 +2339,18 @@ static int request_proxy_anew(REQUEST *request)
}
/*
- * Now that we have a new Id, free the old one
+ * Now that we have a new Id, free the old one (if any)
* and update the various statistics.
*/
PTHREAD_MUTEX_LOCK(&proxy_mutex);
- fr_packet_list_yank(proxy_list, &old);
- fr_packet_list_id_free(proxy_list, &old);
- old_home->currently_outstanding--;
+ if (old_hash) {
+ fr_packet_list_yank(proxy_list, &old);
+ fr_packet_list_id_free(proxy_list, &old);
+ old_home->currently_outstanding--;
#ifdef WITH_TCP
- if (listener) listener->count--;
+ if (listener) listener->count--;
#endif
+ }
PTHREAD_MUTEX_UNLOCK(&proxy_mutex);
/*
-
List info/subscribe/unsubscribe? See http://www.freeradius.org/list/users.html
