On Tuesday, May 3, 2016, Philippe Mouawad <[email protected]> wrote:
> > > On Monday, May 2, 2016, sebb <[email protected] > <javascript:_e(%7B%7D,'cvml','[email protected]');>> wrote: > >> On 2 May 2016 at 20:48, Philippe Mouawad <[email protected]> >> 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 <[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. >> > > > -- > Cordialement. > Philippe Mouawad. > > > > -- Cordialement. Philippe Mouawad.
