Hi,
This patch fixes the multiple concurrency failure in fastcgi (against
rev. 128). Alo's previous patch sent to the list also included in this
patch (Yes, Alo, you were right. It is better to have integer, not the
pointer). The cause of the failure was because the manager relied on
fcgi->id sent by the handler:
in read_fcgi():
return cherokee_fcgi_manager_step (fcgim, fcgi);
In fact, all data that received by the manager from the server have
nothing to do with the current fcgi handler. Error happened under
certain circumstance when the manager received END_QUERY for fcgi->id =
X, but the current fcgi->id in the manager was Y. Hence, the manager
will set the fcgi->phase of Y to 'finished' while it should not be (it
should be X that is set to finished, not Y). Then, the Y manager found
out that it should be finished while the incoming_buffer is still empty.
This patch fixes the problem by resolving the current fcgi->id based on
the record found inside the data coming from the server instead of
getting it from the handler. Therefore, the manager could set the phase
and fill the incoming_buffer for the correct (fcgi->id) handler.
After applying the patch, there is still a problem:
I could never get 'ab -c 1 -n 1000' completed successfully because
exactly in 501th request, the php interpreter always crashed (probably
out of our scope, ie. PHP/libfcgi bug)
Btw, some amusements:
Concurrency Level: 1000
Time taken for tests: 0.878809 seconds
Complete requests: 300
Failed requests: 0
Write errors: 0
Total transferred: 64078 bytes
HTML transferred: 1194 bytes
Requests per second: 341.37 [#/sec] (mean)
Time per request: 2929.363 [ms] (mean)
Time per request: 2.929 [ms] (mean, across all concurrent
requests)
Transfer rate: 70.55 [Kbytes/sec] received
Index: fcgi_manager.c
===================================================================
--- fcgi_manager.c (revision 128)
+++ fcgi_manager.c (working copy)
@@ -361,36 +361,35 @@
return ret_ok;
}
+static cherokee_handler_fastcgi_t *
+get_handler_ref (cherokee_fcgi_manager_t *fcgim)
+{
+ cherokee_handler_fastcgi_t *fcgi = NULL;
+ cherokee_connection_t *conn;
+ conn = fcgim->conn_poll [fcgim->request_id - 1];
+ if (conn == NULL) {
+ SHOULDNT_HAPPEN;
-/* static void */
-/* set_status (cherokee_fcgi_manager_t *fcgim, cherokee_fcgi_status_t status) */
-/* { */
-/* cherokee_handler_fastcgi_t *fcgi; */
-/* cherokee_connection_t *conn; */
-
-/* conn = fcgim->conn_poll [fcgim->request_id - 1]; */
-/* if (conn != NULL) { */
-/* fcgi = (cherokee_handler_fastcgi_t *) conn->handler; */
-/* if (fcgi != NULL) { */
-/* fcgi->status = status; */
-/* } */
-/* } */
-/* } */
+ return NULL;
+ }
+ fcgi = FCGI(conn->handler);
+ if (fcgi == NULL) {
+ SHOULDNT_HAPPEN;
+ return NULL;
+ }
+
+ return fcgi;
+}
+
static void
-process_buffer (cherokee_fcgi_manager_t *fcgim, void *data, cuint_t data_len)
+process_buffer (cherokee_fcgi_manager_t *fcgim, cherokee_handler_fastcgi_t *fcgi, void *data, cuint_t data_len)
{
cherokee_connection_t *conn;
- cherokee_handler_fastcgi_t *fcgi;
char *message;
-
- conn = fcgim->conn_poll [fcgim->request_id - 1];
- if (conn == NULL) return;
- fcgi = (cherokee_handler_fastcgi_t *) conn->handler;
- if (fcgi == NULL) return;
-
+ conn = fcgim->conn_poll [fcgim->request_id - 1];
switch (fcgim->request_type) {
case FCGI_STDERR:
message = (char*) strndup (data, data_len + 1);
@@ -401,23 +400,22 @@
case FCGI_STDOUT:
cherokee_buffer_add (&fcgi->incoming_buffer, data, data_len);
-/* if (fcgim->remaining_size == 0) */
-/* set_status (fcgim, fcgi_data_available); */
break;
}
}
static ret_t
-process_read_buffer (cherokee_fcgi_manager_t *fcgim, cherokee_handler_fastcgi_t *fcgi)
+process_read_buffer (cherokee_fcgi_manager_t *fcgim)
{
- ret_t ret = ret_eagain;
- cuint_t len;
- cuint_t offset = 0;
- cuint_t bytes_to_move;
- FCGI_Header *header;
- FCGI_EndRequestBody *end_request;
- void *start = fcgim->read_buffer.buf;
+ ret_t ret = ret_eagain;
+ cuint_t len;
+ cuint_t offset = 0;
+ cuint_t bytes_to_move;
+ FCGI_Header *header;
+ FCGI_EndRequestBody *end_request;
+ void *start = fcgim->read_buffer.buf;
+ cherokee_handler_fastcgi_t *fcgi = NULL;
while (fcgim->read_buffer.len > 0)
{
@@ -480,11 +478,14 @@
}
}
+ if ((fcgi = get_handler_ref (fcgim)) == NULL)
+ return ret_error;
+
switch (fcgim->request_type) {
case FCGI_STDERR:
case FCGI_STDOUT:
if (len > 0)
- process_buffer (fcgim, (start + offset), len);
+ process_buffer (fcgim, fcgi, (start + offset), len);
break;
case FCGI_END_REQUEST:
@@ -519,33 +520,15 @@
ret_t
-cherokee_fcgi_manager_step (cherokee_fcgi_manager_t *fcgim, cuint_t id)
+cherokee_fcgi_manager_step (cherokee_fcgi_manager_t *fcgim)
{
ret_t ret;
size_t size = 0;
- cherokee_connection_t *conn;
- cherokee_handler_fastcgi_t *fcgi;
-
+
LOCK;
- conn = fcgim->conn_poll [id - 1];
- if (conn == NULL) {
- SHOULDNT_HAPPEN;
+ TRACE (ENTRIES, "Manager(%p) step in_buf.len=%d\n", fcgim, fcgim->read_buffer.len);
- UNLOCK;
- return ret_error;
- }
-
- fcgi = FCGI(conn->handler);
- if (conn == NULL) {
- SHOULDNT_HAPPEN;
-
- UNLOCK;
- return ret_error;
- }
-
- TRACE (ENTRIES, "Manager(%p) step id=%d, in_buf.len=%d\n", fcgim, id, fcgim->read_buffer.len);
-
/* Read from the FastCGI application
*/
if (fcgim->read_buffer.len < sizeof(FCGI_Header)) {
@@ -567,12 +550,12 @@
return ret_error;
}
- TRACE (ENTRIES, "Manager(%p) readed ID=%d ret=%d size=%d\n", fcgim, id, ret, size);
+ TRACE (ENTRIES, "Manager(%p) readed ret=%d size=%d\n", fcgim, ret, size);
}
/* Process the new chunk
*/
- ret = process_read_buffer (fcgim, fcgi);
+ ret = process_read_buffer (fcgim);
switch (ret) {
case ret_ok:
case ret_eagain:
Index: fcgi_manager.h
===================================================================
--- fcgi_manager.h (revision 128)
+++ fcgi_manager.h (working copy)
@@ -67,7 +67,7 @@
ret_t cherokee_fcgi_manager_register_conn (cherokee_fcgi_manager_t *fcgim, cherokee_connection_t *conn, cuint_t *id);
ret_t cherokee_fcgi_manager_unregister_conn (cherokee_fcgi_manager_t *fcgim, cherokee_connection_t *conn);
-ret_t cherokee_fcgi_manager_step (cherokee_fcgi_manager_t *fcgim, cuint_t);
+ret_t cherokee_fcgi_manager_step (cherokee_fcgi_manager_t *fcgim);
ret_t cherokee_fcgi_manager_send (cherokee_fcgi_manager_t *fcgim, cherokee_buffer_t *info, size_t *sent);
#endif /* CHEROKEE_FCGI_MANAGER_H */
Index: handler_fastcgi.c
===================================================================
--- handler_fastcgi.c (revision 128)
+++ handler_fastcgi.c (working copy)
@@ -196,15 +196,17 @@
#endif
static void
-fixup_padding (cherokee_buffer_t *buf, cuint_t id, FCGI_Header *last_header)
+fixup_padding (cherokee_buffer_t *buf, cuint_t id, cuint_t last_header_offset)
{
cuint_t rest;
cuint_t pad;
char padding[8] = {0, 0, 0, 0, 0, 0, 0, 0};
+ FCGI_Header *last_header;
if (buf->len <= 0)
return;
+ last_header = (FCGI_Header *) (buf->buf + last_header_offset);
rest = buf->len % 8;
pad = 8 - rest;
@@ -221,10 +223,9 @@
static void
-add_env_pair_with_id_and_header_ref (cherokee_buffer_t *buf, cuint_t id,
- char *key, int key_len,
- char *val, int val_len,
- FCGI_Header **req_ref)
+add_env_pair_with_id (cherokee_buffer_t *buf, cuint_t id,
+ char *key, int key_len,
+ char *val, int val_len)
{
FCGI_BeginRequestRecord request;
int len;
@@ -235,12 +236,9 @@
fcgi_build_header (&request.header, FCGI_PARAMS, id, len, 0);
- if (req_ref != NULL)
- *req_ref = buf->buf + buf->len;
+ cherokee_buffer_ensure_size (buf, buf->len + sizeof(FCGI_Header) + key_len + val_len);
+ cherokee_buffer_add (buf, (void *)&request.header, sizeof(FCGI_Header));
- cherokee_buffer_ensure_size (buf, buf->len + key_len + val_len + sizeof(FCGI_Header));
- cherokee_buffer_add (buf, (void *)&request.header, sizeof(FCGI_Header));
-
if (key_len <= 127) {
buf->buf[buf->len++] = key_len;
} else {
@@ -265,15 +263,6 @@
static void
-add_env_pair_with_id (cherokee_buffer_t *buf, cuint_t id,
- char *key, int key_len,
- char *val, int val_len)
-{
- add_env_pair_with_id_and_header_ref (buf, id, key, key_len, val, val_len, NULL);
-}
-
-
-static void
add_env_pair_2_params (void *params[2],
char *key, int key_len,
char *val, int val_len)
@@ -289,7 +278,7 @@
static void
-add_more_env (cherokee_handler_fastcgi_t *fcgi, cherokee_buffer_t *buf, FCGI_Header **last_header)
+add_more_env (cherokee_handler_fastcgi_t *fcgi, cherokee_buffer_t *buf, cuint_t *last_header_offset)
{
cherokee_connection_t *conn;
cherokee_buffer_t buffer = CHEROKEE_BUF_INIT;
@@ -347,10 +336,10 @@
FCGI_PATH_TRANSLATED_VAR, sizeof (FCGI_PATH_TRANSLATED_VAR) - 1,
buffer.buf, buffer.len);
- add_env_pair_with_id_and_header_ref (buf, fcgi->id,
- FCGI_SCRIPT_NAME_VAR, sizeof (FCGI_SCRIPT_NAME_VAR) - 1,
- conn->request.buf, conn->request.len,
- last_header);
+ *last_header_offset = buf->len;
+ add_env_pair_with_id (buf, fcgi->id,
+ FCGI_SCRIPT_NAME_VAR, sizeof (FCGI_SCRIPT_NAME_VAR) - 1,
+ conn->request.buf, conn->request.len);
cherokee_buffer_mrproper (&buffer);
}
@@ -362,10 +351,10 @@
ret_t ret;
FCGI_BeginRequestRecord request;
void *params[2];
+ cuint_t last_header_offset;
cherokee_buffer_t tmp = CHEROKEE_BUF_INIT;
cherokee_buffer_t write_tmp = CHEROKEE_BUF_INIT;
cherokee_connection_t *conn = HANDLER_CONN (fcgi);;
- FCGI_Header *last_header = NULL;
cherokee_connection_parse_args (conn);
@@ -386,8 +375,8 @@
ret = cherokee_cgi_build_basic_env (conn, (cherokee_cgi_set_env_pair_t) add_env_pair_2_params, &tmp, params);
if (unlikely (ret != ret_ok)) return ret;
- add_more_env (fcgi, &write_tmp, &last_header);
- fixup_padding (&write_tmp, fcgi->id, last_header);
+ add_more_env (fcgi, &write_tmp, &last_header_offset);
+ fixup_padding (&write_tmp, fcgi->id, last_header_offset);
cherokee_buffer_add_buffer (&fcgi->write_buffer, &write_tmp);
@@ -529,7 +518,7 @@
/* Read info from the FastCGI
*/
- return cherokee_fcgi_manager_step (fcgim, fcgi->id);
+ return cherokee_fcgi_manager_step (fcgim);
}
@@ -624,7 +613,7 @@
case fcgi_post_send:
TRACE (ENTRIES, "Read from FCGI, phase %s\n", "post send");
- if (! cherokee_buffer_is_empty (&fcgi->write_buffer)) {
+ if (! cherokee_buffer_is_empty (&fcgi->write_buffer)) {
ret = cherokee_fcgi_manager_send (fcgi->manager_ref, &fcgi->write_buffer, &size);
switch (ret) {
@@ -730,7 +719,7 @@
case fcgi_phase_send_header:
TRACE (ENTRIES, "Adding headers, phase %s\n", "send header");
- if (!cherokee_buffer_is_empty (&fcgi->write_buffer)) {
+ if (!cherokee_buffer_is_empty (&fcgi->write_buffer)) {
ret = send_write_buffer (fcgi);
switch (ret) {
case ret_ok:
@@ -761,8 +750,12 @@
RET_UNKNOWN(ret);
}
- if (cherokee_buffer_is_empty (&fcgi->incoming_buffer))
+ if (cherokee_buffer_is_empty (&fcgi->incoming_buffer)) {
+ if (fcgi->phase != fcgi_phase_finished)
+ return ret_eagain;
+
return ret_error;
+ }
/* Look the end of headers
*/
_______________________________________________
Cherokee mailing list
Cherokee@lists.alobbs.com
http://www.alobbs.com/cgi-bin/mailman/listinfo/cherokee
