You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Stefan Fritsch <sf...@sfritsch.de> on 2013/08/04 20:52:00 UTC
r1470679, async write completion, non blocking writes, and mod_ssl
Hi,
I did some testing/reviewing of the ssl/event backport proposal
* core, mod_ssl: Lift the restriction that prevents mod_ssl taking
full advantage of the event MPM. Enable the ability for a module
to reverse the sense of a poll event from a read to a write or
vice versa.
The general idea of the patch is good. Unfortunately, the current
version doesn't have much effect. The problem is that the core output
filter, which does all the decisions about buffering, flushing,
blocking, and async write completion, comes after the ssl output
filter. Therefore core_output_filter only sees encrypted data and
never sees file buckets. Therefore it will only do async write
completion if the encrypted data left to send for a request is less
than THRESHOLD_MAX_BUFFER (64k).
To be more efficient the order of the filter doing the
buffering/flushing decitions and the ssl output filter would have to
be reversed. I haven't looked how this could be achieved. Maybe
mod_ssl would have to do its encryption in a AP_FTYPE_NETWORK filter
that comes after the core_output_filter().
Does it make sense to backport the current state nonetheless? While it
seems likely to me that the current API would be sufficient even if
this issue is fixed, it may be prudent to wait with a backport until
we know how the issue should be fixed exactly.
As a related issue, I have noticed that even without ssl, async write
completion does not work too well at the moment. The reason seems that
EnableSendfile/EnableMMAP default to off/on respectively, and that the
core_output_buffer does not treat MMAP buckets like it would treat
file buckets. AFAICS, there is no facility in apr to create anonymous
mmaps and we can assume that an apr_mmap_t is always backed by a file.
Is this correct? I think on some OSs, mmaping /dev/null gives an
anonymous mapping. Can we ignore that case for the purpose of deciding
how much memory a bucket needs?
Cheers,
Stefan
Re: r1470679, async write completion, non blocking writes, and mod_ssl
Posted by Stefan Fritsch <sf...@sfritsch.de>.
Am Freitag, 16. August 2013, 00:31:01 schrieb Graham Leggett:
> On 15 Aug 2013, at 6:15 PM, Stefan Fritsch <sf...@sfritsch.de> wrote:
> > Do you still have a pointer to that report? I have found the
> > commit
> > notice of the initial commit (r105919) which talks about pipelined
> > requests not working with ssl+mpm_event. But in my tests those
> > worked fine without entering the SSL_ERROR_WANT_READ code path.
>
> This was the original report that introduced the "clogging" concept
> to work around the lack of async support:
> http://osdir.com/ml/dev-httpd/2007-06/msg00054.html
>
> I had investigated this problem after someone had told me at a
> conference that the reason mod_ssl didn't work in the event mpm was
> because openssl couldn't support async SSL, which isn't true.
>
> I asked for clarification here:
> http://comments.gmane.org/gmane.comp.apache.devel/50310
Thanks for the pointers. FTR, the issue with pipelined requests I
mentioned above seems to have been fixed by r533820 and r535879.
> > Any test that actually executes the new code would be fine. From
> > the discussion so far, it seems to me like nobody actually did
> > that so far. But I don't see any problem with my comment. The
> > backport proposal was made only one week after commit to trunk.
> > Considering the subtleties involved in that area, and that it now
> > took quite some discussion to determine what is and what is not
> > the scope of the patch, I still think that a rushed backport
> > would have been wrong.
> Can you clarify what subtleties there were in this area? Without
> explicit async support mod_ssl would certainly hang if httpd's core
> polled for read when openssl had expected a write.
The whole code is rather complex. And as you wrote in one of your
other mails, writing instead of read does not seem to be implemented
in mod_ssl. But I have now voted +1. Should we maybe add a log message
to all these bio methods that "should never be called" according to
the comments? And implement the missing ones to log a message, too?
This may make it easier to debug any issues found by normal users. If
yes, at what log level? So high that is is triggered during normal
operations or at some trace level?
> > And
> > pquerna commented on IRC that the solution seemed wrong to him,
> > but
> > unfortunately it appears he didn't have time to look at the patch
> > in detail.
>
> We need something more specific than "it seems wrong". If there is a
> genuine problem I want us to find it, but we can't remain blocked
> indefinitely by a problem that hasn't been defined anywhere.
Yes. As I said, that was one of the reasons for my comment one week
after the commit. It is certainly no longer a valid argument.
Re: r1470679, async write completion, non blocking writes, and mod_ssl
Posted by Graham Leggett <mi...@sharp.fm>.
On 15 Aug 2013, at 6:15 PM, Stefan Fritsch <sf...@sfritsch.de> wrote:
> Do you still have a pointer to that report? I have found the commit
> notice of the initial commit (r105919) which talks about pipelined
> requests not working with ssl+mpm_event. But in my tests those worked
> fine without entering the SSL_ERROR_WANT_READ code path.
This was the original report that introduced the "clogging" concept to work around the lack of async support: http://osdir.com/ml/dev-httpd/2007-06/msg00054.html
I had investigated this problem after someone had told me at a conference that the reason mod_ssl didn't work in the event mpm was because openssl couldn't support async SSL, which isn't true.
I asked for clarification here: http://comments.gmane.org/gmane.comp.apache.devel/50310
> Any test that actually executes the new code would be fine. From the
> discussion so far, it seems to me like nobody actually did that so
> far. But I don't see any problem with my comment. The backport
> proposal was made only one week after commit to trunk. Considering the
> subtleties involved in that area, and that it now took quite some
> discussion to determine what is and what is not the scope of the
> patch, I still think that a rushed backport would have been wrong.
Can you clarify what subtleties there were in this area? Without explicit async support mod_ssl would certainly hang if httpd's core polled for read when openssl had expected a write.
To enable async behaviour in SSL you need to enable it using BIO_set_nbio(). Having enabled this openssl will ask permission before write-during-read or read-during-write instead of just performing the read or write in a blocking fashion. This allows you to poll for the correct event, write or read.
http://linux.die.net/man/3/bio_set_nbio
"BIO_set_nbio() sets the non blocking I/O flag to n. If n is zero then blocking I/O is set. If n is 1 then non blocking I/O is set. Blocking I/O is the default. The call to BIO_set_nbio() should be made before the connection is established because non blocking I/O is set during the connect process."
> And
> pquerna commented on IRC that the solution seemed wrong to him, but
> unfortunately it appears he didn't have time to look at the patch in
> detail.
We need something more specific than "it seems wrong". If there is a genuine problem I want us to find it, but we can't remain blocked indefinitely by a problem that hasn't been defined anywhere.
Regards,
Graham
--
Re: r1470679, async write completion, non blocking writes, and mod_ssl
Posted by Stefan Fritsch <sf...@sfritsch.de>.
Am Donnerstag, 15. August 2013, 10:45:25 schrieb Graham Leggett:
> On 15 Aug 2013, at 09:56, Stefan Fritsch <sf...@sfritsch.de> wrote:
> > I have understood that. But I would have liked to see the sense
> > code in action, but failed to trigger it. At least
> > t/ssl/pr12355.t in the test suite uses renegotiation, and I have
> > also tried client initiated renegotiation (after removing the
> > code that rejects it), but neither causes httpd to use the new
> > code paths. So, do you have a test setup where the new code paths
> > are actually used?
>
> The original report was of a hang, and the cause of the hang was
> clear - OpenSSL async behavior was switched on but the sense
> reversal was ignored. The fix was to complete the functionality. By
> committing it to trunk I hoped that those who had seen the hang
> would be able to verify the hang was gone, as I struggled to
> reproduce it.
Do you still have a pointer to that report? I have found the commit
notice of the initial commit (r105919) which talks about pipelined
requests not working with ssl+mpm_event. But in my tests those worked
fine without entering the SSL_ERROR_WANT_READ code path.
> What didn't help at all was the message in STATUS that more testing
> was needed, as it didn't elaborate what kind of problem or
> behaviour people should be testing for.
Any test that actually executes the new code would be fine. From the
discussion so far, it seems to me like nobody actually did that so
far. But I don't see any problem with my comment. The backport
proposal was made only one week after commit to trunk. Considering the
subtleties involved in that area, and that it now took quite some
discussion to determine what is and what is not the scope of the
patch, I still think that a rushed backport would have been wrong. And
pquerna commented on IRC that the solution seemed wrong to him, but
unfortunately it appears he didn't have time to look at the patch in
detail.
Re: r1470679, async write completion, non blocking writes, and mod_ssl
Posted by Graham Leggett <mi...@sharp.fm>.
On 15 Aug 2013, at 09:56, Stefan Fritsch <sf...@sfritsch.de> wrote:
> I have understood that. But I would have liked to see the sense code
> in action, but failed to trigger it. At least t/ssl/pr12355.t in the
> test suite uses renegotiation, and I have also tried client initiated
> renegotiation (after removing the code that rejects it), but neither
> causes httpd to use the new code paths. So, do you have a test setup
> where the new code paths are actually used?
The original report was of a hang, and the cause of the hang was clear - OpenSSL async behavior was switched on but the sense reversal was ignored. The fix was to complete the functionality. By committing it to trunk I hoped that those who had seen the hang would be able to verify the hang was gone, as I struggled to reproduce it.
What didn't help at all was the message in STATUS that more testing was needed, as it didn't elaborate what kind of problem or behaviour people should be testing for.
One possible reason for not triggering the one path could be that mod_ssl implements read and write channels in one direction, but only read channels in the other (or vice versa, code isn't in front of me). This would need to be fixed before mod_ssl could properly support a non-request-response protocol, and again, this is a separate problem. This limitation doesn't justify implementing only half of the async behavior.
Regards,
Graham
--
Re: r1470679, async write completion, non blocking writes, and mod_ssl
Posted by Stefan Fritsch <sf...@sfritsch.de>.
Am Donnerstag, 15. August 2013, 02:36:25 schrieb Graham Leggett:
> On 14 Aug 2013, at 22:43, Stefan Fritsch <sf...@sfritsch.de> wrote:
> > Unfortunately, I haven't been able to trigger the new code path in
> > mod_ssl being actually used. Do you have any example
> > setup/situation, where the SSL_ERROR_WANT_READ case is actually
> > hit?
>
> I suspect you have misunderstood the problem the patch tries to fix.
>
> SSL negotiation and renegotiation involve both reads and writes, and
> these reads might be performed inside SSL_write, and these writes
> might be performed inside SSL_read.
>
> When OpenSSL is switched to async mode, it returns two distinct
> codes to signify that the sense of the poll must be changed.
> Historically the core was oblivious to this requirement, and
> happily tried to poll for read when OpenSSL had asked for
> permission to write. With nothing to be read, the connection would
> hang indefinitely.
>
> This patch fixed two things. It taught the core how to respect the
> sense requested by OpenSSL, and it removed the hack that forced SSL
> connections to be sync only.
I have understood that. But I would have liked to see the sense code
in action, but failed to trigger it. At least t/ssl/pr12355.t in the
test suite uses renegotiation, and I have also tried client initiated
renegotiation (after removing the code that rejects it), but neither
causes httpd to use the new code paths. So, do you have a test setup
where the new code paths are actually used?
Re: r1470679, async write completion, non blocking writes, and mod_ssl
Posted by Graham Leggett <mi...@sharp.fm>.
On 14 Aug 2013, at 22:43, Stefan Fritsch <sf...@sfritsch.de> wrote:
> Unfortunately, I haven't been able to trigger the new code path in
> mod_ssl being actually used. Do you have any example setup/situation,
> where the SSL_ERROR_WANT_READ case is actually hit?
I suspect you have misunderstood the problem the patch tries to fix.
SSL negotiation and renegotiation involve both reads and writes, and these reads might be performed inside SSL_write, and these writes might be performed inside SSL_read.
When OpenSSL is switched to async mode, it returns two distinct codes to signify that the sense of the poll must be changed. Historically the core was oblivious to this requirement, and happily tried to poll for read when OpenSSL had asked for permission to write. With nothing to be read, the connection would hang indefinitely.
This patch fixed two things. It taught the core how to respect the sense requested by OpenSSL, and it removed the hack that forced SSL connections to be sync only.
What this patch does not attempt to do is optimize the filters to switch to write completion mode earlier on in the response. That is a completely separate problem requiring a separate fix. That problem is probably as simple to fix as teaching mod_ssl to detect when an EOS bucket is present in the brigade, hopefully but not necessarily behind a file bucket, and if so, set aside the brigade and switch on write completion. Next time round, serve from the set aside brigade in write completion mode until done.
Regards,
Graham
--
Re: r1470679, async write completion, non blocking writes, and mod_ssl
Posted by Stefan Fritsch <sf...@sfritsch.de>.
Am Montag, 5. August 2013, 22:50:05 schrieb Stefan Fritsch:
> > Agreed, but from what I can see the proposed patch does
> > add some benefit, allows for future improvement, adds no
> > overhead and no bugs. As such, even though it doesn't completely
> > solve the whole issue, it is valuable enough to be folded into
> > 2.4. (imo 'natch)
>
> It gives us a new API that we need to keep. But if you and Graham
> both think that this is a good tradeoff, then that's fine with me.
> But I want to test one more thing before I vote +1. Hopefully I
> will have some time for that next week-end.
Unfortunately, I haven't been able to trigger the new code path in
mod_ssl being actually used. Do you have any example setup/situation,
where the SSL_ERROR_WANT_READ case is actually hit?
Re: r1470679, async write completion, non blocking writes, and mod_ssl
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Aug 5, 2013, at 4:50 PM, Stefan Fritsch <sf...@sfritsch.de> wrote:
>
> It gives us a new API that we need to keep.
I think it's a useful API, but ymmv
Re: r1470679, async write completion, non blocking writes, and mod_ssl
Posted by Stefan Fritsch <sf...@sfritsch.de>.
Am Montag, 5. August 2013, 09:57:16 schrieb Jim Jagielski:
> On Aug 5, 2013, at 4:00 AM, Stefan Fritsch <sf...@sfritsch.de> wrote:
> > An ideal solution would put the buffering/decision for
> > blocking/non-blocking into ap_pass_brigade(). This way other
> > filters like deflate could also be called asynchronously. But I
> > am not too optimistic that this can be achieved without API
> > changes.
>
> Agreed, but from what I can see the proposed patch does
> add some benefit, allows for future improvement, adds no
> overhead and no bugs. As such, even though it doesn't completely
> solve the whole issue, it is valuable enough to be folded into
> 2.4. (imo 'natch)
It gives us a new API that we need to keep. But if you and Graham both
think that this is a good tradeoff, then that's fine with me. But I
want to test one more thing before I vote +1. Hopefully I will have
some time for that next week-end.
The wording of CHANGES and the docs need to be adjusted, though.
Re: r1470679, async write completion, non blocking writes, and mod_ssl
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Aug 5, 2013, at 4:00 AM, Stefan Fritsch <sf...@sfritsch.de> wrote:
>
> An ideal solution would put the buffering/decision for
> blocking/non-blocking into ap_pass_brigade(). This way other filters like
> deflate could also be called asynchronously. But I am not too optimistic
> that this can be achieved without API changes.
>
Agreed, but from what I can see the proposed patch does
add some benefit, allows for future improvement, adds no
overhead and no bugs. As such, even though it doesn't completely
solve the whole issue, it is valuable enough to be folded into
2.4. (imo 'natch)
Re: r1470679, async write completion, non blocking writes, and
mod_ssl
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Mon, 5 Aug 2013, Graham Leggett wrote:
> Are you seeing a specific problem?
Well, when I download a large file over a slow link, the request does not
enter write completion state but rather the worker thread is still hogged
for (nearly) the entire download.
> The way openssl's async behaviour works, is that is the middle of a
> read, openssl might need to write, and in the middle of a write, openssl
> might need to read. Openssl tells you this through the codes
> SSL_ERROR_WANT_READ and SSL_ERROR_WANT_WRITE. You receive these codes,
> re-run the select or poll or whatever with the sense that openssl has
> asked for, and you're done. It is that simple.
AFAICT, that part is ok. But it doesn't solve the problem that one thread
is hogged for every ssl connection.
> Sure, httpd might do all sorts of buffering of reads and writes, but at
> the end of the day it won't matter, because openssl will never attempt
> to write-during read or read-during-write without asking you whether it
> is ok first.
>
> I can see a potential problem if the core decided to buffer a write, but
> in theory that could be solved by mod_ssl simply sending a flush bucket
> down the stack whenever the sense changes. I don't see why the core
> needs to care that the data is a file, or encrypted, or whatever, the
> core should just do what the core does.
Let me recap the way async works in mpm event: There is no way to
interrupt and resume the handler, so the handler always must run to
completion. The only way async write completion can work is if the data
produced by the handler is buffered and is later sent bit by bit in an
async way. However, this cannot be done unconditionally or the memory
usage would grow to infinity. Therefore the core output filter has some
heuristics on when to buffer data for async write completion and when to
do blocking writes instead. In general, if there is more than 64k of data
in memory, this is written to the network in a blocking way. Only if the
buckets are backed by files (and therefore do not use significant memory
before being sent to the network), more than 64k of request data is put
into async write completion.
This is where mod_ssl comes in. It will read the file buckets, encrypt
them, and then we have encrypted data in memory that will cause the core
output filters to do blocking writes. The blocking writes happen while the
handler does ap_pass_brigade() and therefore before the worker thread
which executes the handler is freed.
An ideal solution would put the buffering/decision for
blocking/non-blocking into ap_pass_brigade(). This way other filters like
deflate could also be called asynchronously. But I am not too optimistic
that this can be achieved without API changes.
Re: r1470679, async write completion, non blocking writes, and mod_ssl
Posted by Graham Leggett <mi...@sharp.fm>.
On 04 Aug 2013, at 8:52 PM, Stefan Fritsch <sf...@sfritsch.de> wrote:
> Hi,
>
> I did some testing/reviewing of the ssl/event backport proposal
>
> * core, mod_ssl: Lift the restriction that prevents mod_ssl taking
> full advantage of the event MPM. Enable the ability for a module
> to reverse the sense of a poll event from a read to a write or
> vice versa.
>
> The general idea of the patch is good. Unfortunately, the current
> version doesn't have much effect. The problem is that the core output
> filter, which does all the decisions about buffering, flushing,
> blocking, and async write completion, comes after the ssl output
> filter. Therefore core_output_filter only sees encrypted data and
> never sees file buckets. Therefore it will only do async write
> completion if the encrypted data left to send for a request is less
> than THRESHOLD_MAX_BUFFER (64k).
>
> To be more efficient the order of the filter doing the
> buffering/flushing decitions and the ssl output filter would have to
> be reversed. I haven't looked how this could be achieved. Maybe
> mod_ssl would have to do its encryption in a AP_FTYPE_NETWORK filter
> that comes after the core_output_filter().
>
> Does it make sense to backport the current state nonetheless? While it
> seems likely to me that the current API would be sufficient even if
> this issue is fixed, it may be prudent to wait with a backport until
> we know how the issue should be fixed exactly.
Are you seeing a specific problem?
The way openssl's async behaviour works, is that is the middle of a read, openssl might need to write, and in the middle of a write, openssl might need to read. Openssl tells you this through the codes SSL_ERROR_WANT_READ and SSL_ERROR_WANT_WRITE. You receive these codes, re-run the select or poll or whatever with the sense that openssl has asked for, and you're done. It is that simple.
Sure, httpd might do all sorts of buffering of reads and writes, but at the end of the day it won't matter, because openssl will never attempt to write-during read or read-during-write without asking you whether it is ok first.
I can see a potential problem if the core decided to buffer a write, but in theory that could be solved by mod_ssl simply sending a flush bucket down the stack whenever the sense changes. I don't see why the core needs to care that the data is a file, or encrypted, or whatever, the core should just do what the core does.
Regards,
Graham
--