jmeter-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Philippe Mouawad <philippe.moua...@gmail.com>
Subject Re: Regression in 3.0 / Bug 59401 / Possible solutions
Date Tue, 03 May 2016 01:45:05 GMT
On Tuesday, May 3, 2016, Philippe Mouawad <philippe.mouawad@gmail.com>
wrote:

>
>
> On Monday, May 2, 2016, sebb <sebbaz@gmail.com
> <javascript:_e(%7B%7D,'cvml','sebbaz@gmail.com');>> wrote:
>
>> 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?
>>
>> The response size in the HttpHc4Impl class
>
This note is wrong I think

>
>
>> >
>> >    - 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.
>
>
>> 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.
>
>
>>
>> >    - 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
>
>> And it does not need to save everything.
>> The unpacker would need to check for the values.
>
>
> can you clarify ?
>
ok understood

>
>> 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> 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.
>>
>
>
> --
> Cordialement.
> Philippe Mouawad.
>
>
>
>

-- 
Cordialement.
Philippe Mouawad.

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message