[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Fwd: [Bug 62590] performance drop after moving from apache 2.2 to apache 2.4

On 08/29/2018 09:37 AM, Christophe Jaillet wrote:
> Pure speculation:
> Actually we ERR_clear_error unconditionally so that in case of error, we can safely call SSL_get_error.
> Doc says:
> <<<<<<<<<< >>>>>>>>>>
> ERR_get_error() returns the earliest error code from the thread's error queue and removes the entry. This function can be called repeatedly until there are no more error codes to return.
> ERR_peek_error() returns the earliest error code from the thread's error queue without modifying it.
> ERR_peek_last_error() returns the latest error code from the thread's error queue without modifying it.
> <<<<<<<<<< >>>>>>>>>>
> Couldn't we avoid the ERR_clear_error call (which looks expensive according to the thread), and:
>    - loop on SSL_get_error to empty the error queue, in case an error occurred and we want la latest one?
> or
>    - do a ERR_peek_last_error() / ERR_clear_error() in the error handling path (if we really care about clearing the error queue)
> IMHO, these 2 solutions would do the same as the current code, without requiring ERR_clear_error in the normal case.

Unfortunately it isn't that easy, because SSL_get_error does not only look at the error stack but also at the return
code from a read / write call passed to it. It quickly breaks out with two possible error return values if something
is on the error stack and this what caused the addition of the ERR_clear_error call. We had the situation that another
consumer of openssl in the code base left an error unhandled on the stack and thus caused SSL_get_error to draw the
wrong conclusions. While we could argue the error is in the other part of the code and the other consumer of openssl
should clear up the error stack correctly I don't think this is the complete solution, because

1. We should not fail if a another consumer is bogus. We should be prepared for others to have bugs regarding this.
2. As the error handling is a stack I think it is a valid use case for another consumer of openssl to check for the
   error later and not directly like e.g. with errno.

I am not sure yet if it is not SSL_get_error that makes wrong assumptions about the error stack, but that would be
something hard to solve for us.