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.
