On 07/03/2019 11:13 PM, Yann Ylavic wrote:
> On Wed, Jul 3, 2019 at 6:38 PM Ruediger Pluem <rpl...@apache.org> wrote:
>>
>> On 07/03/2019 03:46 PM, ic...@apache.org wrote:
>>>
>>> - if (pending == OK) {
>>> + if (pending == OK || (pending == DECLINED &&
>>> + cs->pub.sense == CONN_SENSE_WANT_READ)) {
>>
>> Sorry for being devils advocate here and I may be completely off, but are we
>> sure that
>> cs->pub.sense is still CONN_SENSE_WANT_READ when we get here in all cases?
>
> If CONN_SENSE_WANT_READ is set by the core or a module, it won't
> change "inside" the MPM until after READ/WRITE_COMPLETION has been
> handled, i.e. precisely in this "if {" block where it's reset to
> default.
>
>> If my memory is correct the cs->pub.sense with CONN_SENSE_WANT_READ /
>> CONN_SENSE_WANT_WRITE
>> was introduced as HTTP state might be writing or reading, but the underlying
>> SSL needs just the
>> opposite due to the state the SSL protocol is in.
>
> Frankly I don't see how the CONN_SENSE_WANT_READ set in
> ssl_filter_write (i.e. ssl_io_filter_output) can ever reach MPM event
> in any meaningful way. The mod_ssl output filter runs quite late in
> the connection processing, well after the handshake anyway, and any
> error raised from there leads to the connection to be closed (EAGAIN
> is not something we handle in the output filtering stack).
> If we wanted the MPM to wait for readability during the SSL handshake
> then ssl_hook_process_connection would have to return something else
> than DECLINED in any case, e.g. OK when its ap_get_brigade() =>
> ssl_io_filter_input() => ssl_io_filter_handshake() => SSL_accept()
> returns EAGAIN (with CONN_STATE_WRITE_COMPLETION and
> CONN_SENSE_WANT_READ/WRITE set appropriately, see attached trunk/2.4.x
> patches).
>
> Thus for me the current CONN_SENSE_WANT_READ is useless so this change
> (r1862475) can't break it (and looks like the right thing to do to
> handle CONN_SENSE_WANT_READ in MPM event).
>
>> To be honest I did not have the time to dig deeper, but without further
>> research it might happen
>> that mod_ssl flips this around to CONN_SENSE_WANT_WRITE.
>> More than happy to be proven wrong :-)
>
> I'm not 100% sure to be right but it looks like CONN_SENSE_* semantics
> have gone with the different places we have handled SSL handshake in
> time..
Thanks for the investigation. Wouldn't that bio_filter_out_flush in the patch
cause a blocking write, although we were
non blocking?
Regards
Rüdiger
