Hi Jacob,
[CCing dev@, probably more insight about this from there]
On Mon, Oct 19, 2015 at 6:35 PM, Jacob Champion <champio...@gmail.com> wrote:
> The patchset I recently folded into mod_websocket [1] rails the CPU when
> using ws:// instead of wss://. The problem appears to be that an empty
> non-blocking read from ap_get_brigade() returns EAGAIN when using SSL, but
> without SSL it is returning SUCCESS with an empty brigade.
>
> Yann, I noticed that you wrote about something similar a while ago [2] but I
> don't know if that conversation went anywhere. Is SUCCESS with an empty
> brigade really a correct postcondition for ap_get_brigade(), or is this a
> bug?
The httpd input filters/handlers usually check for both
APR_STATUS_IS_EAGAIN() or SUCCESS+APR_BRIGADE_EMPTY()...
The latter comes from the core input filter, with the following code
and comment:
/* We should treat EAGAIN here the same as we do for EOF (brigade is
* empty). We do this by returning whatever we have read. This may
* or may not be bogus, but is consistent (for now) with EOF logic.
*/
if (APR_STATUS_IS_EAGAIN(rv) && block == APR_NONBLOCK_READ) {
rv = APR_SUCCESS;
}
return rv;
This comment is about AP_MODE_GETLINE, and I think it predates the way
we handle APR_EOF now, with either:
/* ...
* Ideally, this should be returning SUCCESS with EOS bucket, but
* some higher-up APIs (spec. read_request_line via ap_rgetline)
* want an error code.
*/
if (APR_BRIGADE_EMPTY(ctx->b)) {
return APR_EOF;
}
or for AP_MODE_READBYTES/AP_MODE_SPECULATIVE:
if (block == APR_BLOCK_READ && len == 0) {
/* We wanted to read some bytes in blocking mode. We read
* 0 bytes. Hence, we now assume we are EOS.
*
* When we are in normal mode, return an EOS bucket to the
* caller.
* When we are in speculative mode, leave ctx->b empty, so
* that the next call returns an EOS bucket.
*/
[...]
if (mode == AP_MODE_READBYTES) {
e = apr_bucket_eos_create(f->c->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(b, e);
}
return APR_SUCCESS;
}
or else for AP_MODE_READBYTES/AP_MODE_SPECULATIVE still, but non-blocking:
rv = apr_bucket_read(e, &str, &len, block);
if (APR_STATUS_IS_EAGAIN(rv) && block == APR_NONBLOCK_READ) {
/* getting EAGAIN for a blocking read is an error; for a
* non-blocking read, return an empty brigade. */
return APR_SUCCESS;
}
else if (rv != APR_SUCCESS) {
return rv;
}
[else: try to consume all the buckets non-blocking until
the requested number of bytes is reached,
or we would block,
or EOF]
where e is typically (soon or later) the socket bucket (which will
never return APR_EOF on read, but morph to an empty string instead).
So there are indeed some weirdness here, and probably some room for
simplification and optimization.
For the simplification, I agree we should return EAGAIN for a
non-blocking read which would block, thus simplify several callers
which already check it (they have to, as you said, because of mod_ssl)
along with the empty case.
Not to mention that EAGAIN can also be returned in blocking mode (per
the above code) and is to be considered an error, hence the typical
would-block check is currently mode == APR_NONBLOCK_READ &&
(APR_STATUS_IS_EAGAIN(rv) || EMPTY(bb)) (not really simple...), so
this case should be turned to a real error IMO.
We could also return EAGAIN in AP_MODE_GETLINE, but here we would have
to take care of not returning an incomplete line (though this is not
the case currently, since we return SUCCESS in this case too, while we
could easily keep it buffered in ctx->b).
For the optimization, they are also cases where we could return EOF
early (in non-blocking mode), and save some round trips.
There is no way we can return anything else than EOF on the next calls anyway.
I'll (re)take a look at the thread [2] you mentioned (and refresh my
mind, I don't remember all the details :), and then propose something
to dev@...
Regards,
Yann.
