On Monday, May 2, 2016, sebb <[email protected]> wrote: > On 2 May 2016 at 20:48, Philippe Mouawad <[email protected] > <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 <[email protected] <javascript:;>> > wrote: > > > >> On 1 May 2016 at 22:28, Philippe Mouawad <[email protected] > <javascript:;>> > >> wrote: > >> > On Sunday, May 1, 2016, sebb <[email protected] <javascript:;>> wrote: > >> > > >> >> On 1 May 2016 at 22:14, Philippe Mouawad <[email protected] > <javascript:;> > >> >> <javascript:;>> wrote: > >> >> > On Sunday, May 1, 2016, sebb <[email protected] <javascript:;> > <javascript:;>> wrote: > >> >> > > >> >> >> On 1 May 2016 at 21:27, 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 21:12, Philippe Mouawad < > >> [email protected] <javascript:;> > >> >> <javascript:;> > >> >> >> <javascript:;> > >> >> >> >> <javascript:;>> wrote: > >> >> >> >> > On Sunday, May 1, 2016, sebb <[email protected] > <javascript:;> <javascript:;> > >> >> <javascript:;> > >> >> >> <javascript:;>> wrote: > >> >> >> >> > > >> >> >> >> >> On 1 May 2016 at 20:53, Philippe Mouawad < > >> >> [email protected] <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.
