You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by rj...@apache.org on 2007/09/03 00:50:41 UTC
svn commit: r572181 - in /tomcat/connectors/trunk/jk/native/common:
jk_ajp12_worker.c jk_ajp_common.c jk_jni_worker.c jk_status.c
Author: rjung
Date: Sun Sep 2 15:50:40 2007
New Revision: 572181
URL: http://svn.apache.org/viewvc?rev=572181&view=rev
Log:
- Document return codes of the service() method for
all worker types (lb still missing).
Below is for jk_ajp_common.c:
- Document return codes of ajp_send_request() and
ajp_get_reply().
- Fix log message typo
- Don't use the recovery_options for idempotent GET
and HEAD in case we already sent the headers.
- We don't need to handle JK_FATAL_ERROR for
ajp_process_callback() in ajp_get_reply().
- Handle JK_INTERNAL_ERROR instead of JK_SERVER_ERROR
for ajp_process_callback() in ajp_get_reply().
- Add some logging for unexpected default case in
return code handling.
- Move complete handling of return codes of
ajp_send_request() in front of return code handling
for ajp_get_reply() in service().
- Some XXXs still open: return codes, is_error
and op->recoverable for unexpected cases.
We still need a better design for returning enough
information from service() of a member to an lb,
such that the lb can decide about recovery and if the
member should be put in error state.
Modified:
tomcat/connectors/trunk/jk/native/common/jk_ajp12_worker.c
tomcat/connectors/trunk/jk/native/common/jk_ajp_common.c
tomcat/connectors/trunk/jk/native/common/jk_jni_worker.c
tomcat/connectors/trunk/jk/native/common/jk_status.c
Modified: tomcat/connectors/trunk/jk/native/common/jk_ajp12_worker.c
URL: http://svn.apache.org/viewvc/tomcat/connectors/trunk/jk/native/common/jk_ajp12_worker.c?rev=572181&r1=572180&r2=572181&view=diff
==============================================================================
--- tomcat/connectors/trunk/jk/native/common/jk_ajp12_worker.c (original)
+++ tomcat/connectors/trunk/jk/native/common/jk_ajp12_worker.c Sun Sep 2 15:50:40 2007
@@ -84,6 +84,19 @@
static int ajpv12_handle_request(ajp12_endpoint_t * p,
jk_ws_service_t *s, jk_logger_t *l);
+/*
+ * Return values of service() method for ajp12 worker:
+ * return value is_error reason
+ * JK_FALSE JK_HTTP_SERVER_ERROR Invalid parameters (null values)
+ * Error during connect to the backend
+ * ajpv12_handle_request() returns false:
+ * Any error during reading a request body from the client or
+ * sending the request to the backend
+ * JK_FALSE JK_HTTP_OK ajpv12_handle_response() returns false:
+ * Any error during reading parts of response from backend or
+ * sending to client
+ * JK_TRUE JK_HTTP_OK All other cases
+ */
static int JK_METHOD service(jk_endpoint_t *e,
jk_ws_service_t *s,
jk_logger_t *l, int *is_error)
Modified: tomcat/connectors/trunk/jk/native/common/jk_ajp_common.c
URL: http://svn.apache.org/viewvc/tomcat/connectors/trunk/jk/native/common/jk_ajp_common.c?rev=572181&r1=572180&r2=572181&view=diff
==============================================================================
--- tomcat/connectors/trunk/jk/native/common/jk_ajp_common.c (original)
+++ tomcat/connectors/trunk/jk/native/common/jk_ajp_common.c Sun Sep 2 15:50:40 2007
@@ -1178,6 +1178,19 @@
*
* nb: reqmsg is the original request msg buffer
* repmsg is the reply msg buffer which could be scratched
+ *
+ * Return values of ajp_send_request() function:
+ * return value op->recoverable reason
+ * JK_FALSE JK_FALSE/JK_TRUE ajp_connection_tcp_send_message() returns JK_FATAL_ERROR
+ * Endpoint belongs to unknown protocol.
+ * XXX: The recoverability handling of JK_FATAL_ERROR returning from
+ * ajp_connection_tcp_send_message() seems to be inconsistent.
+ * JK_FALSE JK_TRUE ajp_connection_tcp_send_message() returns JK_FALSE
+ * Sending data in jk_tcp_socket_sendfull() returns with error.
+ * JK_FALSE JK_TRUE Could not connect to backend
+ * JK_SERVER_ERROR JK_TRUE Error during sending parts of POST body to backend
+ * JK_CLIENT_RD_ERROR JK_FALSE Error during reading parts of POST body from client
+ * JK_TRUE JK_TRUE All other cases
*/
static int ajp_send_request(jk_endpoint_t *e,
jk_ws_service_t *s,
@@ -1464,7 +1477,7 @@
return JK_AJP13_ERROR;
}
r->http_response_status = res.status;
- rc = is_http_status_fail(ae->worker, res.status);
+ rc = is_http_status_fail(ae->worker, res.status);
if (rc > 0) {
JK_TRACE_EXIT(l);
return JK_STATUS_FATAL_ERROR;
@@ -1543,7 +1556,7 @@
}
jk_log(l, JK_LOG_INFO,
- "Reding from client aborted or client network problems");
+ "Reading from client aborted or client network problems");
JK_TRACE_EXIT(l);
return JK_CLIENT_RD_ERROR;
@@ -1603,6 +1616,26 @@
* ajp13/ajp14 is async but handling read/send this way prevent nice recovery
* In fact if tomcat link is broken during upload (browser -> apache -> tomcat)
* we'll loose data and we'll have to abort the whole request.
+ *
+ * Return values of ajp_get_reply() function:
+ * return value op->recoverable reason
+ * JK_REPLY_TIMEOUT ?recovery_options Reply timeout while waiting for response packet
+ * JK_FALSE ?recovery_options Error during ajp_connection_tcp_get_message()
+ * Communication error or wrong packet content while reading from backend.
+ * JK_STATUS_ERROR mostly JK_TRUE ajp_process_callback() returns JK_STATUS_ERROR
+ * Recoverable, if callback didn't return with a JK_HAS_RESPONSE before.
+ * JK_HAS_RESPONSE: parts of the post buffer are consumed.
+ * JK_STATUS_FATAL_ERROR mostly JK_TRUE ajp_process_callback() returns JK_STATUS_FATAL_ERROR
+ * Recoverable, if callback didn't return with a JK_HAS_RESPONSE before.
+ * JK_HAS_RESPONSE: parts of the post buffer are consumed.
+ * JK_SERVER_ERROR ? ajp_process_callback() returns JK_AJP13_ERROR
+ * JK_AJP13_ERROR: protocol error.
+ * JK_CLIENT_RD_ERROR ? ajp_process_callback() returns JK_CLIENT_RD_ERROR
+ * JK_CLIENT_RD_ERROR: could not read post from client.
+ * JK_CLIENT_WR_ERROR ? ajp_process_callback() returns JK_CLIENT_WR_ERROR
+ * JK_CLIENT_WR_ERROR: could not write back result to client
+ * JK_TRUE ? ajp_process_callback() returns JK_AJP13_END_RESPONSE
+ * JK_FALSE ? Other unhandled cases (unknown return codes)
*/
static int ajp_get_reply(jk_endpoint_t *e,
jk_ws_service_t *s,
@@ -1629,26 +1662,25 @@
if (headeratclient == JK_FALSE) {
if (p->worker->recovery_opts & RECOVER_ABORT_IF_TCGETREQUEST)
op->recoverable = JK_FALSE;
+ /*
+ * We revert back to recoverable, if recovery_opts allow it for GET or HEAD
+ */
+ if (op->recoverable == JK_FALSE) {
+ if (p->worker->recovery_opts & RECOVER_ALWAYS_HTTP_HEAD) {
+ if (!strcmp(s->method, "HEAD"))
+ op->recoverable = JK_TRUE;
+ }
+ else if (p->worker->recovery_opts & RECOVER_ALWAYS_HTTP_GET) {
+ if (!strcmp(s->method, "GET"))
+ op->recoverable = JK_TRUE;
+ }
+ }
}
else {
if (p->worker->recovery_opts & RECOVER_ABORT_IF_TCSENDHEADER)
op->recoverable = JK_FALSE;
}
- /*
- * We revert back to recoverable, if recovery_opts allow it for GET or HEAD
- */
- if (op->recoverable == JK_FALSE) {
- if (p->worker->recovery_opts & RECOVER_ALWAYS_HTTP_HEAD) {
- if (!strcmp(s->method, "HEAD"))
- op->recoverable = JK_TRUE;
- }
- else if (p->worker->recovery_opts & RECOVER_ALWAYS_HTTP_GET) {
- if (!strcmp(s->method, "GET"))
- op->recoverable = JK_TRUE;
- }
- }
-
JK_TRACE_EXIT(l);
return JK_REPLY_TIMEOUT;
}
@@ -1786,15 +1818,6 @@
JK_TRACE_EXIT(l);
return JK_SERVER_ERROR;
}
- else if (JK_FATAL_ERROR == rc) {
- /*
- * we won't be able to gracefully recover from this so
- * set recoverable to false and get out.
- */
- op->recoverable = JK_FALSE;
- JK_TRACE_EXIT(l);
- return JK_FALSE;
- }
else if (JK_CLIENT_RD_ERROR == rc) {
/*
* Client has stop sending to us, so get out.
@@ -1811,18 +1834,29 @@
JK_TRACE_EXIT(l);
return JK_CLIENT_WR_ERROR;
}
- else if (JK_SERVER_ERROR == rc) {
+ else if (JK_INTERNAL_ERROR == rc) {
/*
- * Tomcat has stop talking to us, so get out.
- * Loadbalancer if present will decide if
- * failover is possible.
+ * Internal error, like memory allocation or invalid packet lengths.
*/
JK_TRACE_EXIT(l);
return JK_SERVER_ERROR;
}
+ else if (JK_AJP13_NO_RESPONSE == rc) {
+ /*
+ * This is fine, loop again, more data to send.
+ */
+ }
else if (rc < 0) {
+ /*
+ * XXX: We should disable reuse in this case to.
+ * Maybe use another return value and let service() do the disable reuse.
+ */
+ op->recoverable = JK_FALSE;
+ jk_log(l, JK_LOG_ERROR,
+ "(%s) Callback returns with unknown value %d",
+ p->worker->name, rc);
JK_TRACE_EXIT(l);
- return (JK_FALSE); /* XXX error */
+ return JK_FALSE;
}
}
/* XXX: Not reached? */
@@ -1836,6 +1870,28 @@
*
* We serve here the request, using AJP13/AJP14 (e->proto)
*
+ * Return values of service() method for ajp13/ajp14 worker:
+ * return value is_error op->recoverable reason
+ * JK_FALSE JK_HTTP_SERVER_ERROR UNDEF Invalid Parameters (null values)
+ * JK_SERVER_ERROR JK_HTTP_OK UNDEF Error during initializing empty request, response or post body objects
+ * JK_CLIENT_ERROR JK_REQUEST_TOO_LARGE JK_TRUE Request doesn't fit into buffer (error during ajp_marshal_into_msgb())
+ * JK_CLIENT_ERROR JK_HTTP_BAD_REQUEST JK_FALSE Error during reading parts of POST body from client
+ * JK_SERVER_ERROR JK_HTTP_SERVER_ERROR JK_FALSE If ajp_send_request() returns JK_TRUE but !op->recoverable.
+ * This should never happen.
+ * JK_CLIENT_ERROR JK_HTTP_BAD_REQUEST ? ajp_get_reply() returns JK_CLIENT_RD_ERROR
+ * JK_CLIENT_ERROR JK_HTTP_OK ? ajp_get_reply() returns JK_CLIENT_WR_ERROR
+ * JK_SERVER_ERROR JK_HTTP_SERVER_ERROR ? ajp_get_reply() returns JK_SERVER_ERROR
+ * JK_SERVER_ERROR: protocol error or internal error
+ * JK_REPLY_TIMEOUT JK_HTTP_GATEWAY_TIME_OUT JK_FALSE ajp_get_reply() returns JK_REPLY_TIMEOUT
+ * Only if !op->recoverable
+ * JK_FALSE JK_HTTP_GATEWAY_TIME_OUT JK_FALSE ajp_get_reply() returns something else
+ * Only if !op->recoverable
+ * JK_REPLY_TIMEOUT JK_HTTP_GATEWAY_TIME_OUT JK_TRUE ajp_get_reply() returns JK_REPLY_TIMEOUT
+ * Only if op->recoverable and no more ajp13/ajp14 direct retries
+ * JK_STATUS_ERROR JK_STATUS_ERROR JK_TRUE ajp_get_reply() returns JK_STATUS_ERROR
+ * Only if op->recoverable and no more ajp13/ajp14 direct retries
+ * JK_FALSE JK_HTTP_SERVER_BUSY JK_TRUE ajp_get_reply() returns JK_FALSE or JK_STATUS_FATAL_ERROR
+ * Only if op->recoverable and no more ajp13/ajp14 direct retries
*/
static int JK_METHOD ajp_service(jk_endpoint_t *e,
jk_ws_service_t *s,
@@ -1948,20 +2004,39 @@
* to next valid tomcat.
*/
err = ajp_send_request(e, s, l, p, op);
- if (err == JK_TRUE) {
-
- /* If we have an unrecoverable error, it's probably because
- * the sender (browser) stopped sending data before the end
- * (certainly in a big post)
+ if (err == JK_CLIENT_RD_ERROR) {
+ *is_error = JK_HTTP_BAD_REQUEST;
+ if (p->worker->recovery_opts & RECOVER_ABORT_IF_CLIENTERROR) {
+ /* Mark the endpoint for shutdown */
+ p->reuse = JK_FALSE;
+ }
+ jk_log(l, JK_LOG_INFO,
+ "(%s) sending request to tomcat failed, "
+ "because of client read error "
+ "without recovery in send loop attempt=%d",
+ p->worker->name, i);
+ JK_TRACE_EXIT(l);
+ return JK_CLIENT_ERROR;
+ }
+ else if (err == JK_FALSE) {
+ jk_log(l, JK_LOG_INFO,
+ "(%s) sending request to tomcat failed, "
+ "recoverable operation attempt=%d",
+ p->worker->name, i);
+ }
+ else if (err == JK_TRUE) {
+ /* XXX: this should never happen:
+ * ajp_send_request() never returns JK_TRUE if !op->recoverable.
*/
if (!op->recoverable) {
*is_error = JK_HTTP_SERVER_ERROR;
jk_log(l, JK_LOG_ERROR,
"(%s) sending request to tomcat failed "
+ "because of an unknown reason "
"without recovery in send loop %d",
p->worker->name, i);
JK_TRACE_EXIT(l);
- return JK_FALSE;
+ return JK_SERVER_ERROR;
}
/* Up to there we can recover */
@@ -2032,7 +2107,6 @@
* operation is no more recoverable
*/
if (!op->recoverable) {
- *is_error = JK_HTTP_BAD_GATEWAY;
jk_log(l, JK_LOG_ERROR,
"(%s) receiving reply from tomcat failed "
"without recovery in send loop attempt=%d",
@@ -2042,6 +2116,7 @@
JK_TRACE_EXIT(l);
return JK_REPLY_TIMEOUT;
}
+ *is_error = JK_HTTP_BAD_GATEWAY;
JK_TRACE_EXIT(l);
return JK_FALSE;
}
@@ -2055,39 +2130,19 @@
}
}
}
- if (err == JK_CLIENT_RD_ERROR) {
- *is_error = JK_HTTP_BAD_REQUEST;
- if (p->worker->recovery_opts & RECOVER_ABORT_IF_CLIENTERROR) {
- /* Mark the endpoint for shutdown */
- p->reuse = JK_FALSE;
- }
- jk_log(l, JK_LOG_INFO,
- "(%s) sending request to tomcat failed, "
- "because of client read error "
- "without recovery in send loop attempt=%d",
- p->worker->name, i);
- JK_TRACE_EXIT(l);
- return JK_CLIENT_ERROR;
- }
- else if (err == JK_CLIENT_WR_ERROR) {
- *is_error = JK_HTTP_OK;
- if (p->worker->recovery_opts & RECOVER_ABORT_IF_CLIENTERROR) {
- /* Mark the endpoint for shutdown */
- p->reuse = JK_FALSE;
- }
- jk_log(l, JK_LOG_INFO,
- "(%s) sending request to tomcat failed, "
- "because of client write error "
- "without recovery in send loop attempt=%d",
- p->worker->name, i);
- JK_TRACE_EXIT(l);
- return JK_CLIENT_ERROR;
- }
else {
- jk_log(l, JK_LOG_INFO,
- "(%s) sending request to tomcat failed, "
- "recoverable operation attempt=%d",
- p->worker->name, i + 1);
+ /*
+ * This should never happen.
+ * XXX: What is a good return value?
+ * XXX: What is a good is_error value?
+ * XXX: Do we need to set recoverable and/or reuse to false?
+ */
+ op->recoverable = JK_FALSE;
+ jk_log(l, JK_LOG_ERROR,
+ "(%s) sending request to tomcat failed with unknown value %d",
+ p->worker->name, err);
+ JK_TRACE_EXIT(l);
+ return JK_FALSE;
}
/* Get another connection from the pool and try again.
* Note: All sockets are probably closed already.
Modified: tomcat/connectors/trunk/jk/native/common/jk_jni_worker.c
URL: http://svn.apache.org/viewvc/tomcat/connectors/trunk/jk/native/common/jk_jni_worker.c?rev=572181&r1=572180&r2=572181&view=diff
==============================================================================
--- tomcat/connectors/trunk/jk/native/common/jk_jni_worker.c (original)
+++ tomcat/connectors/trunk/jk/native/common/jk_jni_worker.c Sun Sep 2 15:50:40 2007
@@ -248,6 +248,14 @@
}
#endif
+/*
+ * Return values of service() method for jni worker:
+ * return value is_error reason
+ * JK_FALSE JK_HTTP_SERVER_ERROR Invalid parameters (null values)
+ * Error during attach to the JNI backend
+ * Error during JNI call
+ * JK_TRUE JK_HTTP_OK All other cases
+ */
static int JK_METHOD service(jk_endpoint_t *e,
jk_ws_service_t *s,
jk_logger_t *l, int *is_error)
Modified: tomcat/connectors/trunk/jk/native/common/jk_status.c
URL: http://svn.apache.org/viewvc/tomcat/connectors/trunk/jk/native/common/jk_status.c?rev=572181&r1=572180&r2=572181&view=diff
==============================================================================
--- tomcat/connectors/trunk/jk/native/common/jk_status.c (original)
+++ tomcat/connectors/trunk/jk/native/common/jk_status.c Sun Sep 2 15:50:40 2007
@@ -2969,6 +2969,12 @@
return JK_FALSE;
}
+/*
+ * Return values of service() method for status worker:
+ * return value is_error reason
+ * JK_FALSE JK_HTTP_SERVER_ERROR Invalid parameters (null values)
+ * JK_TRUE JK_HTTP_OK All other cases
+ */
static int JK_METHOD service(jk_endpoint_t *e,
jk_ws_service_t *s,
jk_logger_t *l, int *is_error)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r572181 - in /tomcat/connectors/trunk/jk/native/common:
jk_ajp12_worker.c jk_ajp_common.c jk_jni_worker.c jk_status.c
Posted by Mladen Turk <mt...@apache.org>.
Rainer Jung wrote:
> Hi Mladen,
>
> I'm fine with rolling back and committing in steps.
No need to do that. Let's just take more care in the future.
It'll be much easier to follow the commits thought.
Regards,
Mladen
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r572181 - in /tomcat/connectors/trunk/jk/native/common:
jk_ajp12_worker.c jk_ajp_common.c jk_jni_worker.c jk_status.c
Posted by Rainer Jung <ra...@kippdata.de>.
Hi Mladen,
I'm fine with rolling back and committing in steps.
Will do later today. Usually I do it like that (see jk_map comits
yesterday), but it got a little late yesterday :(
This was motivated by BZ43229. We partially lost track of the many
return codes of the service() methods of all the different workers and
the helper methods in jk_ajp_common(). We test against unused return
codes, but forgot others. I hope you'll see, that the changes make
sense, after breaking them up into parts, which are easier to track.
After adding the code comments about the used return values, I'm
thinking about the reverse process, i.e.
- discussing after which stages we accept a request as recoverable by
the local worker or/and by an lb (and then check the code against that
definition)
- defining under which conditions an lb should put a worker into error
- defining, which meaning our service return codes have
- checking the bundled is_error
Regards,
Rainer
Mladen Turk wrote:
> Huh, this is one huge single commit :(
> I must say it's really hard to follow something like that, and now I
> see that I'll for sure never do again something like that by myself.
>
> Can we in the future try to make the commits that a single-topic
> related? I know it's additional hassle, but lot less then the one
> when someone really tries to understand the patch.
>
>
> Regards,
> Mladen.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r572181 - in /tomcat/connectors/trunk/jk/native/common:
jk_ajp12_worker.c jk_ajp_common.c jk_jni_worker.c jk_status.c
Posted by Mladen Turk <mt...@apache.org>.
rjung@apache.org wrote:
> Author: rjung
> Date: Sun Sep 2 15:50:40 2007
> New Revision: 572181
>
> URL: http://svn.apache.org/viewvc?rev=572181&view=rev
> Log:
> - Document return codes of the service() method for
> all worker types (lb still missing).
> Below is for jk_ajp_common.c:
> - Document return codes of ajp_send_request() and
> ajp_get_reply().
> - Fix log message typo
> - Don't use the recovery_options for idempotent GET
> and HEAD in case we already sent the headers.
> - We don't need to handle JK_FATAL_ERROR for
> ajp_process_callback() in ajp_get_reply().
> - Handle JK_INTERNAL_ERROR instead of JK_SERVER_ERROR
> for ajp_process_callback() in ajp_get_reply().
> - Add some logging for unexpected default case in
> return code handling.
> - Move complete handling of return codes of
> ajp_send_request() in front of return code handling
> for ajp_get_reply() in service().
> - Some XXXs still open: return codes, is_error
> and op->recoverable for unexpected cases.
> We still need a better design for returning enough
> information from service() of a member to an lb,
> such that the lb can decide about recovery and if the
> member should be put in error state.
>
> Modified:
> tomcat/connectors/trunk/jk/native/common/jk_ajp12_worker.c
> tomcat/connectors/trunk/jk/native/common/jk_ajp_common.c
> tomcat/connectors/trunk/jk/native/common/jk_jni_worker.c
> tomcat/connectors/trunk/jk/native/common/jk_status.c
>
Huh, this is one huge single commit :(
I must say it's really hard to follow something like that, and now I
see that I'll for sure never do again something like that by myself.
Can we in the future try to make the commits that a single-topic
related? I know it's additional hassle, but lot less then the one
when someone really tries to understand the patch.
Regards,
Mladen.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org