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 19:51:12 GMT
Note I don't like the following about my patch:
- It introduces more processing (set.contains and toLowercase) in a part of
JMeter that is highly used

My initial patch was better in terms of processing I think

Regards
Philippe

On Tue, May 3, 2016 at 9:44 PM, Philippe Mouawad <philippe.mouawad@gmail.com
> wrote:

> Hello,
>
> Few notes:
>
>    - I think saving the Headers before any modification is made by HC is
>    a good idea, because we never know what HC does with headers, as a proof
>    this bug. But it can wait 3.1.
>    - I am not fan of adding headers to response through
>    response.setHeader(X_J_METER+name,hdr.getValue());. If for some further
>    development they are used to compute a hash for example.
>
>
> I attached a new patch that follows your notes sebb , or at least what I
> understood.
>
> Regards
>
> Philippe
>
>
> On Tue, May 3, 2016 at 4:26 PM, sebb <sebbaz@gmail.com> wrote:
>
>> 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.
>>
>
>
>
> --
> Cordialement.
> Philippe Mouawad.
>
>
>


-- 
Cordialement.
Philippe Mouawad.

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