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)
- Response size:
- I don't see code fixing the wrong size computation that results
from the replacement of header names
- Backward compatibility:
- As I know this subject is very important for you, I am worried
about impact of this on existing plans
- 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
- I think JMeter users usually expect a behaviour similar to Browser,
so want the response headers as received, not as modified by JMeter
So for now, I prefer the first patch but being its author, I am not fully
neutral :-)
Regards
Philippe
On Mon, May 2, 2016 at 2:44 PM, sebb <[email protected]> wrote:
> On 1 May 2016 at 22:28, Philippe Mouawad <[email protected]>
> wrote:
> > On Sunday, May 1, 2016, sebb <[email protected]> wrote:
> >
> >> On 1 May 2016 at 22:14, Philippe Mouawad <[email protected]
> >> <javascript:;>> wrote:
> >> > On Sunday, May 1, 2016, sebb <[email protected] <javascript:;>> wrote:
> >> >
> >> >> On 1 May 2016 at 21:27, Philippe Mouawad <[email protected]
> >> <javascript:;>
> >> >> <javascript:;>> wrote:
> >> >> > On Sunday, May 1, 2016, sebb <[email protected] <javascript:;>
> >> <javascript:;>> wrote:
> >> >> >
> >> >> >> On 1 May 2016 at 21:12, Philippe Mouawad <
> [email protected]
> >> <javascript:;>
> >> >> <javascript:;>
> >> >> >> <javascript:;>> wrote:
> >> >> >> > On Sunday, May 1, 2016, sebb <[email protected] <javascript:;>
> >> <javascript:;>
> >> >> <javascript:;>> wrote:
> >> >> >> >
> >> >> >> >> On 1 May 2016 at 20:53, Philippe Mouawad <
> >> [email protected] <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.
