[asterisk-dev] [Code Review] 3691: HTTP: Add persistent connection support.

[Matt Jordan]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3691/#review12405
-----------------------------------------------------------



/branches/12/configs/http.conf.sample
<https://reviewboard.asterisk.org/r/3691/#comment22633>

    Per RFC 7230, HTTP implementations SHOULD support persistent connections. While that doesn't mean we should support them by default, it would be nice if we did so.
    
    I'm fine with the default for 12 being to disable persistent connections - keeping the default - but I'd make the default value for session_keep_alive to be 15000 for trunk. That enables persistent connections in trunk, which feels more in line with how our mini HTTP server should behave.



/branches/12/main/http.c
<https://reviewboard.asterisk.org/r/3691/#comment22629>

    Out of memory conditions may be one case where it is appropriate to terminate a connection. This is often the case in AMI as well (and one of the few times when Asterisk may appropriately close an AMI connection due to an error in an AMI action handler).
    
    To put this another way: if the user makes another request over the same connection, are we liable to succeed? Or is it likely to cause yet another problem?



/branches/12/main/http.c
<https://reviewboard.asterisk.org/r/3691/#comment22630>

    A 403 might be the other case where it is appropriate to terminate the connection.



/branches/12/main/http.c
<https://reviewboard.asterisk.org/r/3691/#comment22631>

    Again, out of memory condition.



/branches/12/main/http.c
<https://reviewboard.asterisk.org/r/3691/#comment22632>

    Are bit fields really necessary here?
    
    If we have three or more true/false values, it feels like usage of struct ast_flags may be an appropriate solution.



/branches/12/main/http.c
<https://reviewboard.asterisk.org/r/3691/#comment22634>

    You may want to slap a LOW_MEMORY compile time option around this. 4k is hefty.



/branches/12/main/http.c
<https://reviewboard.asterisk.org/r/3691/#comment22635>

    And the same suggestion here about a LOW_MEMORY option.



/branches/12/main/http.c
<https://reviewboard.asterisk.org/r/3691/#comment22636>

    Probably for the MAX_CONTENT_LENGTH as well



/branches/12/main/http.c
<https://reviewboard.asterisk.org/r/3691/#comment22637>

    Low memory options here as well.
    
    Since this 4k upper limit is used extensively, go ahead and #define it, then use a LOW_MEMORY option to change the #define. That will limit the number of places you have to put conditionals in.



/branches/12/main/http.c
<https://reviewboard.asterisk.org/r/3691/#comment22638>

    I may be missing something, but if httpd_process_request returns non-zero, that looks like it will break the connection in httpd_helper_thread. Wouldn't we want a non-terminal error condition here to return 0?


- Matt Jordan


On June 30, 2014, 9:11 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3691/
> -----------------------------------------------------------
> 
> (Updated June 30, 2014, 9:11 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-23552
>     https://issues.asterisk.org/jira/browse/ASTERISK-23552
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Persistent HTTP connection support is needed due to the increased usage of
> the Asterisk core HTTP transport and the frequency at which REST API calls
> are going to be issued.
> 
> * Add http.conf session_keep_alive option to enable persistent
> connections.
> 
> * Parse and discard optional chunked body extension information and
> trailing request headers.
> 
> * Increased the maximum application/json and
> application/x-www-form-urlencoded body size allowed to 4k.  The previous
> 1k was kind of small.
> 
> * Removed a couple inlined versions of ast_http_manid_from_vars() by
> calling the function.  manager.c:generic_http_callback() and
> res_http_post.c:http_post_callback()
> 
> * Add missing va_end() in ast_ari_response_error().
> 
> * Eliminated unnecessary RAII_VAR() use in http.c:auth_create().
> 
> 
> Diffs
> -----
> 
>   /branches/12/res/res_phoneprov.c 417704 
>   /branches/12/res/res_http_websocket.c 417704 
>   /branches/12/res/res_http_post.c 417704 
>   /branches/12/res/res_ari.c 417704 
>   /branches/12/main/tcptls.c 417704 
>   /branches/12/main/manager.c 417704 
>   /branches/12/main/http.c 417704 
>   /branches/12/include/asterisk/tcptls.h 417704 
>   /branches/12/include/asterisk/http.h 417704 
>   /branches/12/configs/http.conf.sample 417704 
>   /branches/12/UPGRADE.txt 417704 
> 
> Diff: https://reviewboard.asterisk.org/r/3691/diff/
> 
> 
> Testing
> -------
> 
> Added debug messages showing when HTTP connections were opened and closed.
> Performed the following operations:
> http://127.0.0.1:8088/static/ajamdemo.html
> https://127.0.0.1:8088/static/ajamdemo.html
> http://127.0.0.1:8088/static/mantest.html
> https://127.0.0.1:8088/static/mantest.html
> 
> Had sgalarneau perform ARI queries and actions with node.js.
> 
> The opened HTTP connections remained open and processed repeated requests until they idle timed out.
> 
> Performed the following get:
> GET /httpstatus HTTP/1.1
> Connection: close
> 
> Observed that the connection closed as requested when completed.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140701/d9173b26/attachment-0001.html>