jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: Regression in 3.0 / Bug 59401 / Possible solutions
Date Mon, 02 May 2016 21:33:44 GMT
On 2 May 2016 at 20:48, Philippe Mouawad <philippe.mouawad@gmail.com> wrote:
> Hello,
> Thanks for the patch, it is now much clearer for me.
>
> Few notes:
>
>    - Performance: As you know this part of Algorithm is hugely used, so I
>    think there is a performance issue with:
>
>
>    -
>
>       responseHeader.getName().replace(X_J_METER, ""))
>
>       -
>
>       => This part is not optimized. I think it's better to test if
> name startsWith and use substring instead of replace which uses a
> Pattern (compiled every time)
>

The patch was intended as a POC.
I'm sure it can be optimised.

>
>    - Response size:
>       - I don't see code fixing the wrong size computation that results
>       from the replacement of header names

What size computation?

>
>    - Backward compatibility:
>       - As I know this subject is very important for you, I am worried
>       about impact of this on existing plans

I just tried and it does not cause any test failures.
However I have just realised that the headers should be adjusted
earlier, in executeRequest.
I'll create a new patch.

Are you sure that your patch cannot cause any problems?
For example, is it possible that HC adjusts the values of any headers
after they have been saved?
Or HC might create new headers.
Such changes would be lost by your patch as it stands.

>    - Usability :
>       - I am afraid users will compare headers they receive in browser vs
>       the ones in JMeter and will not understand why Content-Length has become
>       X-JMeter-Content-Length, same for the 2 other ones

No, the names are not changed when displayed to the user.

>       - I think JMeter users usually expect a behaviour similar to Browser,
>       so want the response headers as received, not as modified by JMeter

The patch restores the original names before display/storage.
That's what the replace does that you were complaining about.
The X-JMeter names are temporary only.

>
> So for now, I prefer the first patch but being its author, I am not fully
> neutral :-)

I don't care whose patch is used.
Just need to ensure that it is efficient and works.

Note that your patch saves ALL the headers regardless, and requires an
extra interceptor.
Though it would be possible to avoid the extra interceptor  by
overriding ResponseContentEncoding.process as my patch does.
And it does not need to save everything.
The unpacker would need to check for the values.

Unless localContext is very inefficient then a hybrid approach might be best.

i.e. use the ResponseContentEncoding class from my patch (which only
saves if necessary and avoids a second interceptor) but save the
values in LocalContext instead of as headers. Then only use the values
if they are needed.

> Regards
>
> Philippe
>
>
>
> On Mon, May 2, 2016 at 2:44 PM, sebb <sebbaz@gmail.com> wrote:
>
>> On 1 May 2016 at 22:28, Philippe Mouawad <philippe.mouawad@gmail.com>
>> wrote:
>> > On Sunday, May 1, 2016, sebb <sebbaz@gmail.com> wrote:
>> >
>> >> On 1 May 2016 at 22:14, Philippe Mouawad <philippe.mouawad@gmail.com
>> >> <javascript:;>> wrote:
>> >> > On Sunday, May 1, 2016, sebb <sebbaz@gmail.com <javascript:;>>
wrote:
>> >> >
>> >> >> On 1 May 2016 at 21:27, Philippe Mouawad <philippe.mouawad@gmail.com
>> >> <javascript:;>
>> >> >> <javascript:;>> wrote:
>> >> >> > On Sunday, May 1, 2016, sebb <sebbaz@gmail.com <javascript:;>
>> >> <javascript:;>> wrote:
>> >> >> >
>> >> >> >> On 1 May 2016 at 21:12, Philippe Mouawad <
>> philippe.mouawad@gmail.com
>> >> <javascript:;>
>> >> >> <javascript:;>
>> >> >> >> <javascript:;>> wrote:
>> >> >> >> > On Sunday, May 1, 2016, sebb <sebbaz@gmail.com
<javascript:;>
>> >> <javascript:;>
>> >> >> <javascript:;>> wrote:
>> >> >> >> >
>> >> >> >> >> On 1 May 2016 at 20:53, Philippe Mouawad <
>> >> philippe.mouawad@gmail.com <javascript:;>
>> >> >> <javascript:;>
>> >> >> >> <javascript:;>
>> >> >> >> >> <javascript:;>> wrote:
>> >> >> >> >> > Hello,
>> >> >> >> >> > As you know a regression has been reported
on 3.0 related to
>> >> >> >> Compressed
>> >> >> >> >> > responses management.
>> >> >> >> >> >
>> >> >> >> >> > HC4.5.2 differs in its behaviour with 4.2.6,
it removes 3
>> >> headers
>> >> >> >> after
>> >> >> >> >> > uncompressing the response:
>> >> >> >> >> > - Content-Length
>> >> >> >> >> > - Content-Encoding
>> >> >> >> >> > - Content-MD5
>> >> >> >> >> >
>> >> >> >> >> > I attached a fix to Bug 59401 that introduces
a
>> >> >> ResponseInterceptor at
>> >> >> >> >> > first position to save initial Headers.
>> >> >> >> >> > These headers are then used by JMeter to
fill in
>> >> >> >> >> > SampleResult#responseHeaders
>> >> >> >> >> >
>> >> >> >> >> > I don't think the fix can introduce regressions
but your
>> review
>> >> is
>> >> >> >> >> welcome
>> >> >> >> >> > as long as alternative solutions proposals.
>> >> >> >> >> >
>> >> >> >> >> > The drawback I see in this patch is that
it introduces a new
>> >> >> >> >> > ResponseInterceptor and saves Headers in
localContext
>> impacting
>> >> >> >> slightly
>> >> >> >> >> > memory and CPU usage.
>> >> >> >> >> >
>> >> >> >> >> >
>> >> >> >> >> > An alternative solution, would be to modify
slightly
>> >> >> >> >> >
>> >> >> >> >>
>> >> >> >>
>> >> >>
>> >>
>> https://github.com/apache/httpclient/blob/4.5.x/httpclient/src/main/java/org/apache/http/client/protocol/ResponseContentEncoding.java#L142
>> >> >> >> >> > to remove the code that removes the headers.
>> >> >> >> >>
>> >> >> >> >> -1; the headers cannot remain as they are no
longer correct.
>> >> >> >> >>
>> >> >> >> >> But this can break existing test plans that would
use the
>> missing
>> >> >> >> headers
>> >> >> >> > no ?
>> >> >> >> >
>> >> >> >> >> However an alternative might be to copy the original
values to
>> an
>> >> >> >> >> X-prefixed header before removal.
>> >> >> >> >
>> >> >> >> > isn't it strange that JMeter adds headers ?
>> >> >> >> > How users can distinguish between servers headers
and jmeter
>> ones ?
>> >> >> >>
>> >> >> >> X-JMeter-Content-xxx
>> >> >> >>
>> >> >> >> Also JMeter can remove them again before storing the response.
>> >> >> >> They would only be used as temporary storage
>> >> >> >
>> >> >> >
>> >> >> > I don't get the whole picture of what you propose ans how
it
>> >> >> > avoid breaking tests.
>> >> >>
>> >> >> You were proposing to add a ResponseInterceptor that saves the
>> headers
>> >> >> in localContext
>> >> >>
>> >> >> I'm suggesting saving them on the response instead.
>> >> >>
>> >> >> So when processing the response, JMeter looks for
>> >> >>
>> >> >> X-JMeter-Content-xxx
>> >> >> rather than
>> >> >> Content-xxx
>> >> >>
>> >> >> This assumes it knows which reponses have been uncompressed.
>> >> >>
>> >> >> Alternatively, if it cannot find Content-xxx it looks for
>> >> >> X-JMeter-Content-xxx.
>> >> >
>> >> >
>> >> > Doing so it changes response size.
>> >>
>> >> It's easy enough to calculate the response size after the temporary
>> >> headers have been removed.
>> >>
>> >> > I'm afraid of the impacts of this solution and possible regressions.
>> >>
>> >> How would it be different from your patch?
>> >
>> > First because as I don't have the patch, it looks to me more invasive ,
>> so
>> > a patch would make the discussion either useless or at least more simple
>> >
>> >>
>> >> > But a patch would make it clear for me.
>> >>
>> >> It's basically the same as your patch.
>> >> Just the storage method is different.
>> >>
>> >> One reason I proposed this is that it would be a possible option for
>> >> HC to provide the headers.
>> >
>> > I don't get this
>>
>> HC could be enhanced to optionally do what my patch does, i.e. copy
>> the 3 headers to X-headers before they become ex-headers.
>>
>> >> I don't know if that would be acceptable, but if it is, then it would
>> >> be possible to drop the interceptor.
>> >>
>> >> >
>> >> >
>> >> >
>> >> >>
>> >> >> > Could you provide a patch ?
>> >> >> >
>> >> >> > thanks
>> >> >> >
>> >> >> >
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >
>> >> >> >> Thx
>> >> >> >> >
>> >> >> >> >>
>> >> >> >> >> >
>> >> >> >> >> >
>> >> >> >> >> > Regards
>> >> >> >> >> > Philippe
>> >> >> >> >>
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > --
>> >> >> >> > Cordialement.
>> >> >> >> > Philippe Mouawad.
>> >> >> >>
>> >> >> >
>> >> >> >
>> >> >> > --
>> >> >> > Cordialement.
>> >> >> > Philippe Mouawad.
>> >> >>
>> >> >
>> >> >
>> >> > --
>> >> > Cordialement.
>> >> > Philippe Mouawad.
>> >>
>> >
>> >
>> > --
>> > Cordialement.
>> > Philippe Mouawad.
>>
>
>
>
> --
> Cordialement.
> Philippe Mouawad.

Mime
View raw message