`echo` after `echo_read_request_body` seems not to behavior as expected

[duhoobo]

conf snippet as follows:

    location /echo {
        echo "header";
        echo_read_request_body;
        echo_request_body;
        echo "trailer";
    }

Issue a request using curl like:

     curl -i http://127.0.0.1/echo -d "body"

got reply as:

    HTTP/1.1 200 OK
    Server: nginx/1.4.3
    Date: Fri, 03 Jan 2014 16:55:47 GMT
    Content-Type: text/plain
    Transfer-Encoding: chunked
    Connection: keep-alive

    header
    body

And I'm missing the "trailer".

I tried to debug it using gdb, found that the last echo was executed as expected. But not getting the output it generated.

[agentzh]

@duhoobo Yes, according to the current implementation of echo_request_body, the original request body bufs are used directly, which contains a "last buf", terminating the response body stream. A better way is to copy all the bufs (both ngx_chain_t and ngx_buf_t) from r->request_body->bufs (but not the data pointed to by the bufs) and to clear the last_buf flag in our copy.

Will you contribute a patch for this? Thanks!

[duhoobo]

Wow, I have to say, this is a challenge for me. I am now a beginner in Nginx devel world, though I'll give it a shot.

[agentzh]

@duhoobo Great! Here're some hints for you:

1. use ngx_alloc_chain_link to allocate ngx_chain_t instances,
2. use ngx_alloc_buf to allocate ngx_buf_t instances and set them to cl->buf where cl is the ngx_chain_t data created in step 1.
3. use ngx_memcpy to copy the contents of the bufs from r->request_body->bufs to yours.
4. clear the last_buf flag in your ngx_buf_t instances if it is set.
5. output your chain.

[duhoobo]

Finally, with the hints from you and a lot of digging into ngx_http_request_body.c, here is the modified version of ngx_http_echo_exec_echo_request_body (I'd like to upload a patch, but just can't find how to do that):

    ngx_int_t
    ngx_http_echo_exec_echo_request_body(ngx_http_request_t *r,
        ngx_http_echo_ctx_t *ctx)
    {
        ngx_buf_t       *b;
        ngx_chain_t     *cl, *out, *chain;
        ngx_chain_t     **ll;

        if (!r->request_body || !r->request_body->bufs) {
            return NGX_OK;
        }

        out = NULL;
        ll = &out;

        for (cl = r->request_body->bufs; cl; cl = cl->next) {

            chain = ngx_alloc_chain_link(r->pool);
            if (chain == NULL) {
                return NGX_HTTP_INTERNAL_SERVER_ERROR;
            }

            b = ngx_calloc_buf(r->pool);
            if (b == NULL) {
                return NGX_HTTP_INTERNAL_SERVER_ERROR;
            }

            if (cl->buf->in_file) {
                b->in_file = 1;
                b->tag = (ngx_buf_tag_t) &ngx_http_echo_exec_echo_request_body;
                b->file_last = cl->buf->file_last;
                b->file = cl->buf->file;

            } else {
                b->temporary = 1;
                b->tag = (ngx_buf_tag_t) &ngx_http_echo_exec_echo_request_body;
                b->start = cl->buf->pos;
                b->pos = cl->buf->pos;
                b->last = cl->buf->last;
                b->end = cl->buf->end;
            }

            chain->buf = b;
            chain->next = NULL;

            *ll = chain;
            ll = &chain->next;
        }

        return ngx_http_echo_send_chain_link(r, ctx, out);
    }

I tested it only with nignx-1.4.2, and it seems to work just fine.

Please, spare me some time to review it.

And by the way, I'd like to pick your brain about a question:

• What is the sync flag of ngx_buf_t for? I greped the source tree and found it show up only in a few places as well as echo_opcode_echo_sync. But still can't understand its effection.

[duhoobo]

And one more thing, I noticed ngx_http_read_client_request_body will not set last_buf to the last ngx_buf_t if temp_file is used.

I don't know what it'll cause, just not feeling well-grounded.

[agentzh]

@duhoobo Thank you for the patch! I've applied a modified version of your patch to master with some test cases.

To answer your questions:

1. The sync flag in ngx_buf_t is just for intentional empty bufs. Many nginx output filters could create "empty bufs" intentionally to simplify the implementation of streaming filtering algorithm. Nginx will complain on pure "empty bufs" (zero sized and with no flags set).
2. The last_buf flag in r->request_body->bufs is not significant for Nginx core's own usage. So it is an inconsistency for in-file request bodies, but it does not matter for the Nginx core. It did matter for our echo_request_body in ngx_echo but gladly we've fixed this anyway.

[agentzh]

@duhoobo BTW, you can submit a patch by either

1. create a pull request on GitHub: https://help.github.com/articles/creating-a-pull-request or
2. create a patch file manually by using the standard patch command line utility: http://linux.about.com/od/commands/l/blcmdl1_patch.htm

[agentzh]

@duhoobo Forgot to mention that the sync bufs generated in ngx_echo are just for working with output filter modules like ngx_xss: https://github.com/agentzh/xss-nginx-module

[duhoobo]

Thanks. Get it!