[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [serf-dev] Re: [PATCH] Serf crash on spurious data between responses

From: Lieven Govaerts <lgo_at_mobsol.be>
Date: Wed, 14 Jan 2015 13:23:05 +0100

On Wed, Jan 14, 2015 at 12:54 PM, Philip Martin
<philip.martin_at_wandisco.com> wrote:
> Lieven Govaerts <lgo_at_mobsol.be> writes:
>
>> The easiest way is to use existing tests as a starting point.:
>> - Test test_serf_connection_request_create sends 2 requests and responses.
>> This tests sends two pipelined requests, but you can modifiy it to
>> send the 1st request, then run the serf loop, then send the 2nd
>> request.
>> - To add the extra data to the response of the first request, you'll
>> have to create the raw response body.
>> Have a look at test test_request_timeout, line:
>> Respond(WithRawData(RESPONSE_408, strlen(RESPONSE_408)))
>
> I tried to do this but I cannot use the testsuite to trigger the
> problem. Using the earlier script to trigger the problem in svn I see
> the following order of calls:
>
> Breakpoint 4, serf_connection_request_create (conn=0x465590,
> setup=0x7ffff75ec67a <setup_request_cb>, setup_baton=0x469be0)
> at outgoing.c:1728
> 1728 request = create_request(conn, setup, setup_baton,
> (gdb) c
> Continuing.
>
> Breakpoint 1, setup_request (request=0x46a308) at outgoing.c:776
> 776 serf_connection_t *conn = request->conn;
> (gdb)
> Continuing.
>
> Breakpoint 2, read_from_connection (conn=0x465590) at outgoing.c:1142
> 1142 int close_connection = FALSE;
> (gdb)
> Continuing.
>
> Breakpoint 3, destroy_request (request=0x46a308) at outgoing.c:544
> 544 serf_connection_t *conn = request->conn;
> (gdb)
> Continuing.
>
> Breakpoint 4, serf_connection_request_create (conn=0x465590,
> setup=0x7ffff75ec67a <setup_request_cb>, setup_baton=0x469be0)
> at outgoing.c:1728
> 1728 request = create_request(conn, setup, setup_baton,
> (gdb)
> Continuing.
>
> Breakpoint 2, read_from_connection (conn=0x465590) at outgoing.c:1142
> 1142 int close_connection = FALSE;
>
>
> That is: create, setup, read, destroy for the first request leaving
> spurious data on the socket, followed by create, read for the second
> request. It's the call to read before setup that is the problem and
> that doesn't happen with the test below. I think the reason is that in
> the test both requests are created at the start, rather than creating
> the second request after handling the first. The test needs to separate
> the two requests so that one gets handled before the second is created.
> I'm not sure how best to do that.
>
>
> Index: test/test_context.c
> ===================================================================
> --- test/test_context.c (revision 2472)
> +++ test/test_context.c (working copy)
> @@ -968,6 +968,43 @@ static void test_max_keepalive_requests(CuTest *tc
> CuAssertIntEquals(tc, num_requests, tb->handled_requests->nelts);
> }
>
> +#define RESPONSE_200_SPURIOUS "HTTP/1.1 200 OK" CRLF\
> +"Date: Tue, 13 Jan 2015 22:06:51 GMT" CRLF\
> +"Server: Apache/2.2.22" CRLF\
> +"Content-Length: 27" CRLF\
> +"Content-Type: text/html; charset=UTF-8" CRLF \
> +CRLF\
> +"<html><body></body></html> "
> +
> +static void test_spurious_data(CuTest *tc)
> +{
> + test_baton_t *tb = tc->testBaton;
> + apr_status_t status;
> + handler_baton_t handler_ctx[2];
> + const int num_requests = sizeof(handler_ctx)/sizeof(handler_ctx[0]);
> +
> + setup_test_mock_server(tb);
> + status = setup_test_client_context(tb, NULL, tb->pool);
> + CuAssertIntEquals(tc, APR_SUCCESS, status);
> +
> + Given(tb->mh)
> + GETRequest(URLEqualTo("/"), ChunkedBodyEqualTo("1"),
> + HeaderEqualTo("Host", tb->serv_host))
> + Respond(WithRawData(RESPONSE_200_SPURIOUS,
> + strlen(RESPONSE_200_SPURIOUS)))
> + GETRequest(URLEqualTo("/"), ChunkedBodyEqualTo("2"),
> + HeaderEqualTo("Host", tb->serv_host))
> + Respond(WithCode(200), WithChunkedBody(""))
> + EndGiven
> +
> + create_new_request(tb, &handler_ctx[0], "GET", "/", 1);
> + create_new_request(tb, &handler_ctx[1], "GET", "/", 2);
> + serf_connection_set_max_outstanding_requests(tb->connection, 1);
> +
> + run_client_and_mock_servers_loops_expect_ok(tc, tb, num_requests,
> + handler_ctx, tb->pool);
> +}

Can't test now, but I expect it should be something like this:

    create_new_request(tb, &handler_ctx[0], "GET", "/", 1);
    run_client_and_mock_servers_loops_expect_ok(tc, tb, 1,
handler_ctx, tb->pool);

    create_new_request(tb, &handler_ctx[1], "GET", "/", 2);
    run_client_and_mock_servers_loops_expect_ok(tc, tb, 1,
handler_ctx, tb->pool);

I'll have more time this weekend to have a look.

Lieven

> +
> /*****************************************************************************/
> CuSuite *test_context(void)
> {
> @@ -975,6 +1012,7 @@ CuSuite *test_context(void)
>
> CuSuiteSetSetupTeardownCallbacks(suite, test_setup, test_teardown);
>
> +#if 0
> SUITE_ADD_TEST(suite, test_serf_connection_request_create);
> SUITE_ADD_TEST(suite, test_serf_connection_priority_request_create);
> SUITE_ADD_TEST(suite, test_closed_connection);
> @@ -993,6 +1031,9 @@ CuSuite *test_context(void)
> SUITE_ADD_TEST(suite, test_connection_large_response);
> SUITE_ADD_TEST(suite, test_connection_large_request);
> SUITE_ADD_TEST(suite, test_max_keepalive_requests);
> +#else
> + SUITE_ADD_TEST(suite, test_spurious_data);
> +#endif
>
> return suite;
> }
>
> --
> Philip Martin | Subversion Committer
> WANdisco // *Non-Stop Data*
Received on 2015-01-14 13:23:54 CET

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.