Author: rhuijben
Date: Thu Nov 26 15:55:10 2015
New Revision: 1716726
URL: http://svn.apache.org/viewvc?rev=1716726&view=rev
Log:
Make the auth code requeue the same request instance when another request is
needed to get the required response.
Serf 1.3 requeued a different request and destroyed the existing one, which
makes it impossible for end-user applications to perform further operations
on a request. This patch fixes that behavior by rerouting the existing
response to a new request instead.
* auth/auth.c
(serf__handle_auth_response): Use bool. Just handle the headers and let
requeue the request. This avoids work in case we can just reset the
stream. Mark handling done to avoid processing every time during body
handling.
* outgoing_request.c
(clean_resp): Add comment.
(serf__handle_response): Update caller.
(create_request): Use bool arguments.
(serf_connection_request_create): Update caller.
(insert_priority_request): New function, extracted from...
(priority_request_create): ... here.
(discard_response_handler): New function.
(serf__request_requeue): Rewrite as...
(serf_connection__request_requeue): ... this.
* protocols/http2_stream.c
(serf_http2_stream_data_t): Track resetted.
(serf_http2__stream_create): Init value.
(serf_http2__stream_reset): We reset only once.
(serf_http2__stream_cancel_request): Handle request updating and
the remaining work as reset.
(stream_response_eof): Don't return EOF on stream reset, as handlers
will commonly report that as reading errors. Just keep returning
EAGAIN.
(serf_http2__stream_processor): Handle switching requests and
resetting requests.
* serf_private.h
(IOV_MAX): Remove unused variable.
(SERF__STD_IOV_COUNT): Use APR_MAX_IOVEC_SIZE instead of IOV_MAX.
(serf_request_t): Add variable. Use bool for a few others.
(serf__handle_auth_response): Use bool for output argument.
(serf__request_requeue): Remove function.
(serf_connection__request_requeue): New function.
Modified:
serf/trunk/auth/auth.c
serf/trunk/outgoing_request.c
serf/trunk/protocols/http2_stream.c
serf/trunk/serf_private.h
Modified: serf/trunk/auth/auth.c
URL:
http://svn.apache.org/viewvc/serf/trunk/auth/auth.c?rev=1716726&r1=1716725&r2=1716726&view=diff
==============================================================================
--- serf/trunk/auth/auth.c (original)
+++ serf/trunk/auth/auth.c Thu Nov 26 15:55:10 2015
@@ -279,7 +279,7 @@ static apr_status_t dispatch_auth(int co
authentication or validation is needed.
*CONSUMED_RESPONSE will be 1 if authentication is involved (either a 401/407
response or a response with an authn header), 0 otherwise. */
-apr_status_t serf__handle_auth_response(int *consumed_response,
+apr_status_t serf__handle_auth_response(bool *consumed_response,
serf_request_t *request,
serf_bucket_t *response,
apr_pool_t *pool)
@@ -287,7 +287,7 @@ apr_status_t serf__handle_auth_response(
apr_status_t status;
serf_status_line sl;
- *consumed_response = 0;
+ *consumed_response = false;
/* TODO: the response bucket was created by the application, not at all
guaranteed that this is of type response_bucket!! */
@@ -318,25 +318,23 @@ apr_status_t serf__handle_auth_response(
/* Don't bother handling the authentication request if the response
wasn't received completely yet. Serf will call
serf__handle_auth_response
again when more data is received. */
- status = discard_body(response);
- *consumed_response = 1;
-
- /* Discard all response body before processing authentication. */
- if (!APR_STATUS_IS_EOF(status)) {
- return status;
- }
status = dispatch_auth(sl.code, request, response, pool);
if (status != APR_SUCCESS) {
return status;
}
- /* Requeue the request with the necessary auth headers. */
- /* ### application doesn't know about this request! we just drop it
- ### on the floor. */
- (void) serf__request_requeue(request);
+ request->auth_done = true;
+
+ /* Requeue the request with the necessary auth headers.*/
+ status = serf_connection__request_requeue(request);
- return APR_EOF;
+ if (status)
+ return status;
+
+ *consumed_response = true;
+
+ return APR_SUCCESS;
} else {
serf__validate_response_func_t validate_resp;
serf_connection_t *conn = request->conn;
@@ -365,7 +363,7 @@ apr_status_t serf__handle_auth_response(
/* If there was an error in the final step of the authentication,
consider the reponse body as invalid and discard it. */
status = discard_body(response);
- *consumed_response = 1;
+ *consumed_response = true;
if (!APR_STATUS_IS_EOF(status)) {
return status;
@@ -375,6 +373,8 @@ apr_status_t serf__handle_auth_response(
}
}
+ request->auth_done = true;
+
return APR_SUCCESS;
}
Modified: serf/trunk/outgoing_request.c
URL:
http://svn.apache.org/viewvc/serf/trunk/outgoing_request.c?rev=1716726&r1=1716725&r2=1716726&view=diff
==============================================================================
--- serf/trunk/outgoing_request.c (original)
+++ serf/trunk/outgoing_request.c Thu Nov 26 15:55:10 2015
@@ -36,6 +36,11 @@ static apr_status_t clean_resp(void *dat
serf_request_t *request = data;
apr_pool_t *respool = request->respool;
+ /* Note that this function must handle rehooking several
+ variables to a different request!
+
+ See serf_connection__request_requeue() */
+
request->respool = NULL;
/* The request's RESPOOL is being cleared. */
@@ -276,7 +281,7 @@ apr_status_t serf__setup_request(serf_re
apr_status_t serf__handle_response(serf_request_t *request,
apr_pool_t *pool)
{
- int consumed_response = 0;
+ bool consumed_response = false;
/* Only enable the new authentication framework if the program has
* registered an authentication credential callback.
@@ -284,7 +289,7 @@ apr_status_t serf__handle_response(serf_
* This permits older Serf apps to still handle authentication
* themselves by not registering credential callbacks.
*/
- if (request->conn->ctx->cred_cb) {
+ if (!request->auth_done && request->conn->ctx->cred_cb) {
apr_status_t status;
status = serf__handle_auth_response(&consumed_response,
@@ -411,8 +416,8 @@ static serf_request_t *
create_request(serf_connection_t *conn,
serf_request_setup_t setup,
void *setup_baton,
- int priority,
- int ssltunnel)
+ bool priority,
+ bool ssltunnel)
{
serf_request_t *request;
@@ -428,6 +433,7 @@ create_request(serf_connection_t *conn,
request->priority = priority;
request->writing = SERF_WRITING_NONE;
request->ssltunnel = ssltunnel;
+ request->auth_done = FALSE;
request->next = NULL;
request->auth_baton = NULL;
request->protocol_baton = NULL;
@@ -447,8 +453,8 @@ serf_request_t *serf_connection_request_
serf_request_t *request;
request = create_request(conn, setup, setup_baton,
- 0, /* priority */
- 0 /* ssl tunnel */);
+ false /* priority */,
+ false /* ssl tunnel */);
/* Link the request to the end of the request chain. */
serf__link_requests(&conn->unwritten_reqs, &conn->unwritten_reqs_tail,
request);
@@ -460,18 +466,11 @@ serf_request_t *serf_connection_request_
return request;
}
-static serf_request_t *
-priority_request_create(serf_connection_t *conn,
- int ssltunnelreq,
- serf_request_setup_t setup,
- void *setup_baton)
+static void
+insert_priority_request(serf_request_t *request)
{
- serf_request_t *request;
serf_request_t *iter, *prev;
-
- request = create_request(conn, setup, setup_baton,
- 1, /* priority */
- ssltunnelreq);
+ serf_connection_t *conn = request->conn;
/* Link the new request after the last written request. */
iter = conn->unwritten_reqs;
@@ -509,6 +508,21 @@ priority_request_create(serf_connection_
/* Ensure our pollset becomes writable in context run */
serf_io__set_pollset_dirty(&conn->io);
+}
+
+static serf_request_t *
+priority_request_create(serf_connection_t *conn,
+ int ssltunnelreq,
+ serf_request_setup_t setup,
+ void *setup_baton)
+{
+ serf_request_t *request;
+
+ request = create_request(conn, setup, setup_baton,
+ true /* priority */,
+ ssltunnelreq);
+
+ insert_priority_request(request);
return request;
}
@@ -532,16 +546,141 @@ serf_request_t *serf__ssltunnel_request_
setup, setup_baton);
}
-
-serf_request_t *serf__request_requeue(const serf_request_t *request)
+/* Serf request_handler_t to discard a request */
+static apr_status_t discard_response_handler(serf_request_t *request,
+ serf_bucket_t *response,
+ void *handler_baton,
+ apr_pool_t *pool)
{
- /* ### in the future, maybe we could reset REQUEST and try again? */
- return priority_request_create(request->conn,
- request->ssltunnel,
- request->setup,
- request->setup_baton);
+ apr_status_t status;
+ apr_size_t len;
+
+ do
+ {
+ const char *data;
+
+ status = serf_bucket_read(response, SERF_READ_ALL_AVAIL, &data, &len);
+ } while (!status && len);
+
+ return status;
}
+apr_status_t serf_connection__request_requeue(serf_request_t *request)
+{
+ serf_request_t *discard;
+ serf_request_t **pr;
+
+ /* For some reason, somebody wants to restart this request, that was
+ at least partially written and partially read. Most likely to
+ slightly change it. (E.g. to add auth headers).
+
+ To handle this we have to do two things: we have to handle remaining
+ incoming data on the existing request. And we have to make the request
+ ready to write it again.. preferably as the first new request.
+ */
+
+ /* Setup a new request to handle discarding the remaining body */
+ discard = create_request(request->conn,
+ NULL, NULL,
+ request->priority,
+ request->ssltunnel);
+
+ discard->respool = request->respool;
+ discard->allocator = request->allocator;
+ discard->req_bkt = request->req_bkt;
+ discard->resp_bkt = request->resp_bkt;
+
+ discard->setup = NULL; /* Already setup */
+ discard->setup_baton = NULL;
+
+ discard->acceptor = NULL;
+ discard->acceptor_baton = NULL; /* Already accepted */
+
+ discard->handler = discard_response_handler;
+ discard->handler_baton = NULL;
+
+ discard->protocol_baton = request->protocol_baton;
+ discard->auth_done = request->auth_done;
+ discard->auth_baton = request->auth_baton;
+
+ discard->writing = request->writing;
+
+ if (discard->respool) {
+ apr_pool_cleanup_kill(discard->respool, request, clean_resp);
+ apr_pool_cleanup_register(discard->respool, discard,
+ clean_resp, apr_pool_cleanup_null);
+ }
+
+ if (request->depends_on) {
+ /* Update request dependencies */
+ discard->depends_on = request->depends_on;
+
+ for (pr = &discard->depends_on->depends_first;
+ *pr;
+ pr = &(*pr)->depends_next)
+ {
+ if (*pr == request) {
+ *pr = discard;
+ discard->depends_next = request->depends_next;
+ request->depends_next = NULL;
+ request->depends_on = NULL;
+ break;
+ }
+ }
+ }
+ if (request->depends_first) {
+ serf_request_t *r;
+
+ for (r = request->depends_first; r; r = r->depends_next) {
+ r->depends_on = discard;
+ }
+ discard->depends_first = request->depends_first;
+ request->depends_first = NULL;
+ }
+
+ /* And now make the discard request take the place of the old request */
+ for (pr = &request->conn->written_reqs; *pr; pr = &(*pr)->next) {
+ if (*pr == request) {
+ *pr = discard;
+ discard->next = request->next;
+ request->next = NULL;
+
+ if (!discard->next)
+ request->conn->written_reqs_tail = discard;
+ break;
+ }
+ }
+
+ /* Tell the protocol handler (if any) that we are no longer looking at the
+ request */
+
+ if (request->conn->perform_cancel_request)
+ request->conn->perform_cancel_request(discard,
SERF_ERROR_HTTP2_CANCEL);
+
+ /* And now unhook the existing request */
+ request->allocator = NULL;
+ request->respool = NULL;
+ request->req_bkt = NULL;
+ request->resp_bkt = NULL;
+ request->writing = SERF_WRITING_NONE;
+ request->auth_baton = NULL;
+ request->protocol_baton = NULL;
+
+ request->acceptor = NULL;
+ request->acceptor_baton = NULL;
+ request->handler = NULL;
+ request->handler_baton = NULL;
+ request->auth_done = false;
+
+ if (discard->depends_first || discard->depends_on) {
+ serf_connection_request_prioritize(request, discard, 0xFFFF,
+ TRUE);
+ }
+
+ insert_priority_request(request);
+
+ return APR_SUCCESS;
+}
apr_status_t serf_request_cancel(serf_request_t *request)
{
Modified: serf/trunk/protocols/http2_stream.c
URL:
http://svn.apache.org/viewvc/serf/trunk/protocols/http2_stream.c?rev=1716726&r1=1716725&r2=1716726&view=diff
==============================================================================
--- serf/trunk/protocols/http2_stream.c (original)
+++ serf/trunk/protocols/http2_stream.c Thu Nov 26 15:55:10 2015
@@ -37,6 +37,7 @@ struct serf_http2_stream_data_t
serf_bucket_t *response_agg;
serf_hpack_table_t *tbl;
serf_bucket_t *data_tail;
+ bool resetted;
};
serf_http2_stream_t *
@@ -61,6 +62,7 @@ serf_http2__stream_create(serf_http2_pro
stream->data->response_agg = NULL;
stream->data->tbl = NULL;
stream->data->data_tail = NULL;
+ stream->data->resetted = false;
stream->lr_window = lr_window;
stream->rl_window = rl_window;
@@ -430,9 +432,11 @@ serf_http2__stream_reset(serf_http2_stre
{
stream->status = H2S_CLOSED;
- if (stream->streamid < 0)
+ if (stream->streamid < 0 || stream->data->resetted)
return APR_SUCCESS;
+ stream->data->resetted = true;
+
if (local_reset)
return serf_http2__enqueue_stream_reset(stream->h2,
stream->streamid,
@@ -442,8 +446,8 @@ serf_http2__stream_reset(serf_http2_stre
}
void serf_http2__stream_cancel_request(serf_http2_stream_t *stream,
- serf_request_t *rq,
- apr_status_t reason)
+ serf_request_t *rq,
+ apr_status_t reason)
{
if (stream->streamid < 0)
return; /* Never hit the wire */
@@ -456,17 +460,11 @@ void serf_http2__stream_cancel_request(s
reason = SERF_ERROR_HTTP2_CANCEL;
}
+ stream->data->request = rq; /* Might have changed! */
+
/* Let the other party know we don't want anything */
serf_http2__stream_reset(stream, reason, true);
- if (!stream->data)
- return;
-
- if (stream->data && stream->data->request)
- stream->data->request = NULL;
-
- /* Would be nice if we could response_agg, but that is typically
- not safe here, as we might still be reading from it */
}
void serf_http2__stream_prioritize_request(serf_http2_stream_t *stream,
@@ -488,6 +486,9 @@ stream_response_eof(void *baton,
{
serf_http2_stream_t *stream = baton;
+ if (stream->data->resetted)
+ return APR_EAGAIN;
+
switch (stream->status)
{
case H2S_CLOSED:
@@ -750,14 +751,15 @@ serf_http2__stream_processor(void *baton
serf_bucket_t *bucket)
{
serf_http2_stream_t *stream = baton;
+ serf_http2_stream_data_t *sd = stream->data;
apr_status_t status = APR_SUCCESS;
- SERF_H2_assert(stream->data->response_agg != NULL);
+ SERF_H2_assert(stream->data->response_agg != NULL
+ && !stream->data->resetted);
- if (stream->data->request) {
- serf_request_t *request = stream->data->request;
+ if (sd->request) {
- SERF_H2_assert(request->resp_bkt != NULL);
+ SERF_H2_assert(sd->request->resp_bkt != NULL);
/* Response handlers are expected to read until they get some error,
but at least some implementations assume that just returning
@@ -768,36 +770,42 @@ serf_http2__stream_processor(void *baton
some part of the frame open (good case), or we might have completed
the frame and are never called again. */
do {
- status = serf__handle_response(request, request->respool);
+ status = serf__handle_response(sd->request,
+ sd->request->respool);
} while (status == APR_SUCCESS);
- if (!APR_STATUS_IS_EOF(status)
- && !SERF_BUCKET_READ_ERROR(status))
+ if (sd->resetted) {
+ status = APR_EOF;
+ }
+ else if (!APR_STATUS_IS_EOF(status)
+ && !SERF_BUCKET_READ_ERROR(status))
+ {
return status;
+ }
/* Ok, the request thinks is done, let's handle the bookkeeping,
to remove it from the outstanding requests */
{
- serf_connection_t *conn = serf_request_get_conn(request);
+ serf_connection_t *conn = serf_request_get_conn(sd->request);
serf_request_t **rq = &conn->written_reqs;
serf_request_t *last = NULL;
- while (*rq && (*rq != request)) {
+ while (*rq && (*rq != sd->request)) {
last = *rq;
rq = &last->next;
}
if (*rq)
{
- (*rq) = request->next;
+ (*rq) = sd->request->next;
- if (conn->written_reqs_tail == request)
+ if (conn->written_reqs_tail == sd->request)
conn->written_reqs_tail = last;
conn->nr_of_written_reqs--;
}
- serf__destroy_request(request);
+ serf__destroy_request(sd->request);
stream->data->request = NULL;
}
@@ -874,7 +882,7 @@ serf_http2__stream_processor(void *baton
}
}
- if (APR_STATUS_IS_EOF(status)
+ if ((APR_STATUS_IS_EOF(status) || sd->resetted)
&& (stream->status == H2S_CLOSED
|| stream->status == H2S_HALFCLOSED_REMOTE))
{
@@ -883,6 +891,7 @@ serf_http2__stream_processor(void *baton
frames */
serf_bucket_destroy(stream->data->response_agg);
stream->data->response_agg = NULL;
+ status = APR_EOF;
}
return status;
Modified: serf/trunk/serf_private.h
URL:
http://svn.apache.org/viewvc/serf/trunk/serf_private.h?rev=1716726&r1=1716725&r2=1716726&view=diff
==============================================================================
--- serf/trunk/serf_private.h (original)
+++ serf/trunk/serf_private.h Thu Nov 26 15:55:10 2015
@@ -65,16 +65,9 @@ typedef int serf__bool_t; /* Not _Bool *
### stop, rebuild a pollset, and repopulate it. what suckage. */
#define MAX_CONN 16
-/* Windows does not define IOV_MAX, so we need to ensure it is defined. */
-#ifndef IOV_MAX
-/* There is no limit for iovec count on Windows, but apr_socket_sendv
- allocates WSABUF structures on stack if vecs_count <= 50. */
-#define IOV_MAX 50
-#endif
-
/* But we use our own limit in most cases. Typically 50 on Windows
(see IOV_MAX definition above) and typically 64 on posix */
-#define SERF__STD_IOV_COUNT MIN(IOV_MAX, 64)
+#define SERF__STD_IOV_COUNT MIN(APR_MAX_IOVEC_SIZE, 64)
/* Older versions of APR do not have this macro. */
@@ -231,9 +224,10 @@ struct serf_request_t {
serf_bucket_t *resp_bkt;
serf_request_writing_t writing;
- int priority;
+ bool priority;
/* 1 if this is a request to setup a SSL tunnel, 0 for normal requests. */
- int ssltunnel;
+ bool ssltunnel;
+ bool auth_done; /* auth and connection level handling done */
serf_request_t *depends_on; /* On what request do we depend */
serf_request_t *depends_next; /* Next dependency on parent*/
@@ -673,7 +667,7 @@ apr_status_t serf__auth_setup_request(pe
* Handles a 401 or 407 response, tries the different available authentication
* handlers.
*/
-apr_status_t serf__handle_auth_response(int *consumed_response,
+apr_status_t serf__handle_auth_response(bool *consumed_response,
serf_request_t *request,
serf_bucket_t *response,
apr_pool_t *pool);
@@ -719,7 +713,7 @@ apr_status_t serf__provide_credentials(s
apr_pool_t *pool);
/* Requeue a request (at the front). */
-serf_request_t *serf__request_requeue(const serf_request_t *request);
+apr_status_t serf_connection__request_requeue(serf_request_t *request);
apr_status_t serf_connection__perform_setup(serf_connection_t *conn);
