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 Tue, 03 May 2016 14:26:57 GMT
On 3 May 2016 at 02:30, Philippe Mouawad <philippe.mouawad@gmail.com> wrote:
> On Monday, May 2, 2016, sebb <sebbaz@gmail.com> wrote:
>
>> On 2 May 2016 at 20:48, Philippe Mouawad <philippe.mouawad@gmail.com
>> <javascript:;>> 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?
>>
>> The response size in the HttpHc4Impl class
>
>
>> >
>> >    - 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?
>
>
> My patch currently just backups the received headers and uses this backup
> to fill in response headers from SampleResult
> No HC headers are touched.

That's not the point.

The user sees the copies of the headers in the sample result; the HC
object is not kept once the data has been extracted.
Since your patch takes the headers only from the copy which was taken
before the Content-Encoding etc are checked, any subsequent changes to
the headers won't be copied to the sample result.
We already know that HC modifies headers during response processing
(otherwise the patch would not be needed).

I agree it seems unlikely, but since we are only interested in the 3
Content headers, we should just save/restore those.

>
>> Or HC might create new headers.
>> Such changes would be lost by your patch as it stands.
>
> No as I don't interfere with hc created headers.

See above, not relevant.

>
>>
>> >    - 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.
>
> Ok good news
>
>>
>> >       - 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.
>
> Yes
>
>> Though it would be possible to avoid the extra interceptor  by
>> overriding ResponseContentEncoding.process as my patch does.
>
> I didn't want to copy from ResponseContentEncoding

It's a public method.
We don't have to do anything except copy the headers and then call the
parent method (i.e. the extra code in mine could be dropped).
That also guarantees that the code will be run just before the headers
may be dropped.

Since we are overriding the behaviour of the parent class it seems
like the ideal place to do it.

>> And it does not need to save everything.
>> The unpacker would need to check for the values.
>
>
> can you clarify ?

If the saving is conditional, it has to check if the headers have been saved.

>>
>> Unless localContext is very inefficient then a hybrid approach might be
>> best.
>
>
> I don't think it is inefficient as only reference to array is saved but
> this may need checking.
>
>>
>> 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.
>
> yes
>
>
>>
>> > Regards
>> >
>> > Philippe
>> >
>> >
>> >
>> > On Mon, May 2, 2016 at 2:44 PM, sebb <sebbaz@gmail.com <javascript:;>>
>> wrote:
>> >
>> >> On 1 May 2016 at 22:28, Philippe Mouawad <philippe.mouawad@gmail.com
>> <javascript:;>>
>> >> wrote:
>> >> > On Sunday, May 1, 2016, sebb <sebbaz@gmail.com <javascript:;>>
wrote:
>> >> >
>> >> >> On 1 May 2016 at 22:14, 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:27, 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 21:12, Philippe Mouawad <
>> >> philippe.mouawad@gmail.com <javascript:;>
>> >> >> <javascript:;>
>> >> >> >> <javascript:;>
>> >> >> >> >> <javascript:;>> wrote:
>> >> >> >> >> > On Sunday, May 1, 2016, sebb <sebbaz@gmail.com
>> <javascript:;> <javascript:;>
>> >> >> <javascript:;>
>> >> >> >> <javascript:;>> wrote:
>> >> >> >> >> >
>> >> >> >> >> >> On 1 May 2016 at 20:53, Philippe Mouawad
<
>> >> >> philippe.mouawad@gmail.com <javascript:;> <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.
>>
>
>
> --
> Cordialement.
> Philippe Mouawad.

Mime
View raw message