<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/3402/">https://reviewboard.asterisk.org/r/3402/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 27th, 2014, 3:30 p.m. CDT, <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">My only problem with this patch is that I really want to know for sure that there is something low level and buggy happening that requires such a change before approving it. So I have a few questions:
1) I notice you're using both fflush() and shutdown(). If you remove the fflush() does the bug still occur? What about if you add the fflush() but keep the close() in instead of using shutdown()? The reason I ask is that shutdown() and close() are both supposed to send a TCP FIN and flush out the write buffer on its own, so in theory the fflush() call should not be necessary. However, if removing fflush() causes the bug to return, then that leads to my next question:
2) Did this bug manifest itself on many HTTP client implementations or was it only seen on one? When calling close() or shutdown(), if there is data in the socket's write buffer, this data may be sent in the TCP FIN that Asterisk sends. It may be that whatever HTTP client is being used ignores data in TCP FIN and that may be where the real bug lies. It would be interesting to use TCPdump or some tool to monitor the HTTP traffic to see if there is any correlation between the way the HTTP traffic is sent and when the bug manifests itself.
Overall, I'm just not comfortable saying that this fixes a race condition in the kernel since I don't know what due diligence has been done to prove that. On the other hand, I also can't really fault the changes here if they're actually helping improve the situation for an HTTP client. I guess what I'm saying is that the comments and the reasons for the code changes are bothering me more than the actual code changes themselves :)</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">1) The use of fflush is strictly a CYA to insure that the last data from fwrite got pushed by clib to write() prior to the low level shutdown call, and does not resolve the problem on it's own. Nor does removing it cause the failure case to return.
2) The bug manifests itself only under very specific conditions:
a) POST not GET
b) Client is not on same ip/tcp stack/host as Asterisk
c) request coming from json Shred user-agent (unable to duplicate failure using python requests - can't get exact matching request headers)
I used to believe that a virtual machine was a requirement, but it also fails between two physical machines - so long as the client and host have to actually transmit IP packets between them it can fail (even across WiFi and VPN it fails).
Under these circumstances, bug is about 90% failure rate (every now and then the correct body message does arrive). Switching the fwrite() call with the body to a write() does not resolve issue. Using write() to send an extra byte before the body results in that character arriving and not the body. Strangely enough however, writing an extra byte after the body still results in neither arriving (may have to do with length of data written).
Exact test results with original body delivery code and alternate close sequences:
Original code that fails:
fclose(ser->f);
Works:
shutdown(ser->fd,2);
fclose(ser->f);
Fails:
close(ser->fd);
fclose(ser->f);
Fails:
fflush(ser->f);
fclose(ser->f);
Fails:
fflush(ser->f);
close(ser->fd);
Technically, my comments mention a race condition existing without specifying where. Honestly, I'm not entirely certain that it's in the kernel, although it's the most likely culprit at this point.
</pre>
<br />
<p>- Scott</p>
<br />
<p>On March 27th, 2014, 1:52 p.m. CDT, Scott Griepentrog wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By Scott Griepentrog.</div>
<p style="color: grey;"><i>Updated March 27, 2014, 1:52 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-23548">ASTERISK-23548</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">When running asterisk in a virtual machine, responses to ARI requests would frequently be missing. A race condition related to closing the socket immediately after writing data onto it is resolved in this patch by insuring the output stream is flushed, and then informing TCP of the shutdown prior to the close.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Tested on Ubuntu server 12.04 with Sam's json api test from issue.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/12/main/tcptls.c <span style="color: grey">(411242)</span></li>
<li>/branches/12/main/manager.c <span style="color: grey">(411242)</span></li>
<li>/branches/12/main/http.c <span style="color: grey">(411242)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/3402/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>