Helo, while looking at builtin modules' use of ap_get_brigade(), it seems to me that some don't behave correctly with non-blocking calls which would block.
I must say that the core filter's behaviour to return APR_SUCCESS with an
empty brigade, which may be turned to APR_EAGAIN if ap_http_filter() is in
the place, is not abvious, and obliges callers to handle both cases.
The ap_core_input_filter() does :
- for AP_MODE_GETLINE :
/* 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;
}
- for AP_MODE_READBYTES|SPECULATIVE :
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;
}
I don't really see the point with the EOF logic, where is the core filter
returning an empty brigade when EOF? FWICS, it returns either a final EOS
bucket or APR_EOF...
What I can see is that
it will never return APR_EOF if it has some aside buckets (from a previous
call), even if it gets EOF while grabing more buckets (on this call), but
it returns APR_EOF when no
bucket is available, and that's exactly what it should do with APR_EAGAIN,
for consistency (IMHO).
There is a case (described in PR55475, and addressed by
https://issues.apache.org/bugzilla/attachment.cgi?id=30857&action=edit)
where it may return APR_SUCCESS and an empty brigade when EOF, even in
blocking mode, but that's an issue (IMHO).
So, would ap_
core_input_filter() returning APR_EAGAIN when non-blocking *and* no bucket
is available (still returning APR_SUCCESS with aside buckets when
available) break things? be inappropriate?
If not, it would have
the advantage for the caller (via ap_get_brigade)
to check only for rv != APR_SUCCESS (and APR_STATUS_IS_EAGAIN(rv) for
those who care), and a patch could look like the
"httpd-trunk-core_filter_wouldblock.patch" (attached). I guess It really
simplifies things, at least. Note this patch also includes the proposed fix
in PR55475 about ap_core_input_filter() in getline mode and immediate EOF,
so that now ap_core_input_filter() would never return an empty brigade with
APR_SUCCESS (in any case).
If it breaks things, take a look at the second patch
"httpd-trunk-util_filter_wouldblock.patch" (attached), where I added a new
util_filter::ap_got_wouldblock_brigade() function to do the check on
whether the previous ap_get_brigade() was a non-blocking read which would
have blocked, and then use it wherever it can be (ie. a non-blocking read
is or may be asked).
For this second patch, I did not checked whether every code behaved already
well when given an empty brigade, some do, some don't (mod_deflate,
mod_sed, mod_charset_lite, ?), and others I don't know since they handle
empty brigade as EAGAIN or rely (maybe) on ap_http_filter() in the chain to
do the (good) job.
The purpose here is to show that it is really constraining for the coder to
take care (or not) of this check, specially for an input filter likely to
do multiple ap_get_brigade() calls (like mod_deflate, mod_reqtimeout,
mod_sed, mod_proxy_http/connect/wstunnel, ...), without knowledge of the
other filters in the chain.
Any thought?
Regards,
Yann.
httpd-trunk-core_filter_wouldblock.patch
Description: Binary data
httpd-trunk-util_filter_wouldblock.patch
Description: Binary data
