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:30:28 GMT
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.


> 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 ?

>
> 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
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message