You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2006/01/02 16:14:40 UTC
c->aborted is not set correctly on trunk
I just noticed that c->aborted does not get set on trunk when you abort a connection in the browser (with worker mpm,
but I guess with prefork is the same).
This works in 2.2.x. I guess this has something to do with the async write refactoring in the trunk compared
to 2.2.x. In 2.2.x c->aborted gets set in the core output filter whereas in the trunk this has vanished.
Brian could you please have a look into this? As you are familiar with the logic I guess you are faster
then me with this. If you have no time then let me know and I will have a look.
Regards
Rüdiger
Re: c->aborted is not set correctly on trunk
Posted by Rüdiger Plüm <r....@gmx.de>.
On 01/05/2006 05:24 PM, Joe Orton wrote:
> On Thu, Jan 05, 2006 at 08:14:58AM -0800, Justin Erenkrantz wrote:
>
>>I do note that the 2.0 code says:
>>
>> /* The client has aborted, but the request was successful. We
>> * will report success, and leave it to the access and error
>> * logs to note that the connection was aborted.
>> */
>> return APR_SUCCESS;
This comment is also in 2.2.x. I thought that it was there for good reason,
so I implemented this behaviour again in the patch.
>>
>>I'm just not sure I agree with that. -- justin
>
>
> Yes, that little "feature" is a great source of module bugs so it would
> be great to remove it.
>
> The problem it works around is really the fact that the default_handler
> does the bogus trick of returning:
>
> return ap_pass_brigade(r->output_filters, bb);
So I understand that we have two problems here:
1. A non working setting of c->aborted, which should be fixed by the new version
of the patch, which leaves the return codes as they are. I will commit it
soon.
2. The behaviour of at least the default_handler to possibly return an APR
error code if a filter returns non-APR_SUCCESS.
Regards
Rüdiger
Re: c->aborted is not set correctly on trunk
Posted by Joe Orton <jo...@redhat.com>.
On Thu, Jan 05, 2006 at 08:14:58AM -0800, Justin Erenkrantz wrote:
> I do note that the 2.0 code says:
>
> /* The client has aborted, but the request was successful. We
> * will report success, and leave it to the access and error
> * logs to note that the connection was aborted.
> */
> return APR_SUCCESS;
>
> I'm just not sure I agree with that. -- justin
Yes, that little "feature" is a great source of module bugs so it would
be great to remove it.
The problem it works around is really the fact that the default_handler
does the bogus trick of returning:
return ap_pass_brigade(r->output_filters, bb);
which means that you get an APR error code logged as the request status
code if a filter returns non-APR_SUCCESS.
joe
Re: c->aborted is not set correctly on trunk
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On Thu, Jan 05, 2006 at 05:02:54PM +0100, Ruediger Pluem wrote:
>
>
> On 01/02/2006 04:14 PM, Ruediger Pluem wrote:
> > I just noticed that c->aborted does not get set on trunk when you abort a connection in the browser (with worker mpm,
> > but I guess with prefork is the same).
> > This works in 2.2.x. I guess this has something to do with the async write refactoring in the trunk compared
> > to 2.2.x. In 2.2.x c->aborted gets set in the core output filter whereas in the trunk this has vanished.
>
> I had a look into this and created a patch that solved my test case. But as the core output filter is a fundamental
> part of the code I would like to have some remote eyes on it. My approach was to simulate the 'old behaviour'
> in 2.2.x:
>
>
> Index: server/core_filters.c
> ===================================================================
> --- server/core_filters.c�������(Revision 366181)
> +++ server/core_filters.c�������(Arbeitskopie)
> @@ -413,11 +413,12 @@
> if (new_bb == NULL) {
> apr_status_t rv = send_brigade_nonblocking(net->client_socket, bb,
> &(ctx->bytes_written), c);
> - if (APR_STATUS_IS_EAGAIN(rv)) {
> - rv = APR_SUCCESS;
> + if ((rv != APR_SUCCESS) && !APR_STATUS_IS_EAGAIN(rv)) {
> + /* The client aborted the connection */
> + c->aborted = 1;
> }
> setaside_remaining_output(f, ctx, bb, 0, c);
> - return rv;
> + return APR_SUCCESS;
> }
Why are we masking the non-EAGAIN error values too? I'd prefer that we
continue to return the error code for everything but EAGAIN - like the
current code does. (Setting c->aborted here probably does make sense
though.)
I do note that the 2.0 code says:
/* The client has aborted, but the request was successful. We
* will report success, and leave it to the access and error
* logs to note that the connection was aborted.
*/
return APR_SUCCESS;
I'm just not sure I agree with that. -- justin
Re: c->aborted is not set correctly on trunk
Posted by Ruediger Pluem <rp...@apache.org>.
On 01/02/2006 04:14 PM, Ruediger Pluem wrote:
> I just noticed that c->aborted does not get set on trunk when you abort a connection in the browser (with worker mpm,
> but I guess with prefork is the same).
> This works in 2.2.x. I guess this has something to do with the async write refactoring in the trunk compared
> to 2.2.x. In 2.2.x c->aborted gets set in the core output filter whereas in the trunk this has vanished.
I had a look into this and created a patch that solved my test case. But as the core output filter is a fundamental
part of the code I would like to have some remote eyes on it. My approach was to simulate the 'old behaviour'
in 2.2.x:
Index: server/core_filters.c
===================================================================
--- server/core_filters.c»······(Revision 366181)
+++ server/core_filters.c»······(Arbeitskopie)
@@ -413,11 +413,12 @@
if (new_bb == NULL) {
apr_status_t rv = send_brigade_nonblocking(net->client_socket, bb,
&(ctx->bytes_written), c);
- if (APR_STATUS_IS_EAGAIN(rv)) {
- rv = APR_SUCCESS;
+ if ((rv != APR_SUCCESS) && !APR_STATUS_IS_EAGAIN(rv)) {
+ /* The client aborted the connection */
+ c->aborted = 1;
}
setaside_remaining_output(f, ctx, bb, 0, c);
- return rv;
+ return APR_SUCCESS;
}
·
bytes_in_brigade = 0;
@@ -430,7 +431,9 @@
apr_status_t rv = send_brigade_blocking(net->client_socket, bb,
&(ctx->bytes_written), c);
if (rv != APR_SUCCESS) {
- return rv;
+ /* The client aborted the connection */
+ c->aborted = 1;
+ return APR_SUCCESS;
}
bb = remainder;
next = APR_BRIGADE_FIRST(bb);
@@ -464,14 +467,18 @@
apr_status_t rv = send_brigade_blocking(net->client_socket, bb,
&(ctx->bytes_written), c);
if (rv != APR_SUCCESS) {
- return rv;
+ /* The client aborted the connection */
+ c->aborted = 1;
+ return APR_SUCCESS;
}
}
else if (bytes_in_brigade >= THRESHOLD_MIN_WRITE) {
apr_status_t rv = send_brigade_nonblocking(net->client_socket, bb,
&(ctx->bytes_written), c);
if ((rv != APR_SUCCESS) && (!APR_STATUS_IS_EAGAIN(rv))) {
- return rv;
+ /* The client aborted the connection */
+ c->aborted = 1;
+ return APR_SUCCESS;
}
}