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

Tue Jul 1 21:22:15 CDT 2014

> On July 1, 2014, 11:39 a.m., Matt Jordan wrote:
> > /branches/12/main/http.c, lines 1658-1661
> > <https://reviewboard.asterisk.org/r/3691/diff/1/?file=61642#file61642line1658>
> >
> >     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?

This is an error in the processing of the request line of the HTTP request.  The only thing we can do is to drop the connection since we won't know how much of any following data is part of the request.  The return value is irrelevant at this point because the request->close_on_completion is true here.  Until we call http_request_tracking_setup() the only thing we can do on an error is to drop the connection.

> On July 1, 2014, 11:39 a.m., Matt Jordan wrote:
> > /branches/12/configs/http.conf.sample, lines 48-54
> > <https://reviewboard.asterisk.org/r/3691/diff/1/?file=61639#file61639line48>
> >
> >     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.

I'll leave this open as a reminder for when the patch is committed.

> On July 1, 2014, 11:39 a.m., Matt Jordan wrote:
> > /branches/12/main/http.c, lines 309-314
> > <https://reviewboard.asterisk.org/r/3691/diff/1/?file=61642#file61642line309>
> >
> >     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?

Out of memory errors are transitory in a multi-threaded environment.  The system may have more memory available the next time we need some because another thread released it.

That being said, the default Linux optimistic allocator would need to get memory from another process or kill Asterisk if it could not actually get memory for the already "allocated" block.  I'm not sure how useful that allocator really is as far as Asterisk is concerned.  The ao2 objects would tend to demand that the memory be truly allocated immediately.

> On July 1, 2014, 11:39 a.m., Matt Jordan wrote:
> > /branches/12/main/http.c, lines 892-894
> > <https://reviewboard.asterisk.org/r/3691/diff/1/?file=61642#file61642line892>
> >
> >     You may want to slap a LOW_MEMORY compile time option around this. 4k is hefty.

4k isn't that hefty by itself but there are at least three 4k buffers on the stack at this point.

I've defined MAX_HTTP_LINE_LENGTH for this constant to 1k when LOW_MEMORY.  This does reduce the max line a LOW_MEMORY system will handle.

> On July 1, 2014, 11:39 a.m., Matt Jordan wrote:
> > /branches/12/main/http.c, lines 1027-1029
> > <https://reviewboard.asterisk.org/r/3691/diff/1/?file=61642#file61642line1027>
> >
> >     Probably for the MAX_CONTENT_LENGTH as well

LOW_MEMORY drops this constant to 1k as well.  This is a malloced buffer which makes more sense to reduce than the max line length though.

> On July 1, 2014, 11:39 a.m., Matt Jordan wrote:
> > /branches/12/main/http.c, lines 414-419
> > <https://reviewboard.asterisk.org/r/3691/diff/1/?file=61642#file61642line414>
> >
> >     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.

Done.

You lose language support for using the ast_xxx_flag() macros.  And the macros more or less do what the bitfields are going to do unless you work with more than one flag at a time.

- rmudgett

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

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/20140702/19cf8dc8/attachment-0001.html>