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 <[email protected] > 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 <[email protected]> wrote: > >> On 3 May 2016 at 02:30, Philippe Mouawad <[email protected]> >> wrote: >> > 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. >> >> 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 <[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. >> > > > > -- > Cordialement. > Philippe Mouawad. > > > -- Cordialement. Philippe Mouawad.
