Bug 58807

> Both parts could then be controlled via static flags instead of instance
> flags. One flag would be in HTTPHCAbstractImpl and controls reset of the
> SSLContext, one flag would be in HTTPHC4Impl and HTTPHC3Impl and controls
> the HTTPClient resets.

Correction: static -> static ThreadLocal

Created attachment 33410 [details]
Fixing SSLContext/HTTPClient/Connection reuse

The patch fixes reuse/reset between test iterations (on the same thread) of SSLContext, HTTPClient and HTTP connections for HC4 (and likely HC3).

For experiments the reuse of the three objects can be controlled using three different system properties

- reuse.http.connections
- reuse.http.client
- https.use.cached.ssl.context

"true" means "reuse" in all three cases, "false" means "reset/close" (do not reuse).

Resetting the HTTPClient results in also resetting HTTP Connections, but not vice versa.

It is not yet clear to me if we need to support both cases, resetting connection plus HTTPClient and also resetting Connections but reusing HTTPClient, or if one of the twi would suffice.

Feedback welcome.

(In reply to Rainer Jung from comment #2)
> For experiments the reuse of the three objects can be controlled using three
> different system properties

They seem to be JMeter properties, which is better.

Yes, sorry, JMeter properties.

I think we don't need/want all three of them (individual control over connection reuse, HTTPClient reuse and SSLContext reuse). After some experimenting IMHO the useful cases are:

- full reuse of SSLContext, HTTPClient and existing connections
- no reuse of SSLContext, HTTPClient and existing connections

The default should be full reuse like it is today.

The question is though, whether the name "https.use.cached.ssl.context" for the property is good. Once could use a uniform switch to determine reuse in the HTTP case as well, something like reset.http.objects.before.iteration. Default would be "false" (no reset = reuse) and to start each iteration with fresh objects you would switch to "true".

The old flag could be deprecated. If a user would use both, the old and the new, we could warn during startup and the new flag would win.

(In reply to Rainer Jung from comment #4)
> 
> The question is though, whether the name "https.use.cached.ssl.context" for
> the property is good. 

Seems OK to me; the only problem is it does not fully describe what is re-used.
But that could be fixed by better docn in the jmeter.properties file.

> Once could use a uniform switch to determine reuse in
> the HTTP case as well, something like reset.http.objects.before.iteration.

Why do we need the reuse flag for http?

As far as I understand the situation reusing (the default) means e.g. connections are kept open and are reused via HTTP Keep-Alive during the next iteration. I do see this behavior using https, I expect it to happen for http too. That's of course efficient but might not be what you want. A new iteration typically is a new user who would start with new connections.

Of course modelling the connection behavior in an exact way is beyond what we support, but the decision to start a new iteration with new connections could be interesting for some (approximate modeling). It is somehow analogous to modeling the handshake rate by resetting the SSLContext, although a new connection is of cours less disruptive than a new ssl handshake. All in all not a critical feature.

In addition I think the optional behavior "reset instead of reuse" for http and https is a good safety net against problems that might creep in due to reuse of objects that might have more context than we expect (wrong basic auth, wrong cookie, wrong client cert, wrong SNI etc.). Of course most of these things do work now, but reusing the connections and the objects from the HC libs as well as the SSLContext might introduce future bugs of the sort "next iteration uses context data from the previous one" which could then be worked around quickly by disabling the reuse. In general since the risks are quite vague so I'd stay on the performance side here for the default value (reuse). But probably based on my incomplete understanding of the lifecycles of HC objects, I'd feel better if we had such a "reset between iterations" switch.

(In reply to Rainer Jung from comment #6)
> As far as I understand the situation reusing (the default) means e.g.
> connections are kept open and are reused via HTTP Keep-Alive during the next
> iteration. I do see this behavior using https, I expect it to happen for
> http too. That's of course efficient but might not be what you want. A new
> iteration typically is a new user who would start with new connections.
>
> Of course modelling the connection behavior in an exact way is beyond what
> we support, but the decision to start a new iteration with new connections
> could be interesting for some (approximate modeling). It is somehow
> analogous to modeling the handshake rate by resetting the SSLContext,
> although a new connection is of cours less disruptive than a new ssl
> handshake. All in all not a critical feature.

Surelt that's already catered for by the "Use Keepalive" checkbox?
If not checked, JMeter sends Connection: close.
 
> In addition I think the optional behavior "reset instead of reuse" for http
> and https is a good safety net against problems that might creep in due to
> reuse of objects that might have more context than we expect (wrong basic
> auth, wrong cookie, wrong client cert, wrong SNI etc.). Of course most of
> these things do work now, but reusing the connections and the objects from
> the HC libs as well as the SSLContext might introduce future bugs of the
> sort "next iteration uses context data from the previous one" which could
> then be worked around quickly by disabling the reuse. In general since the
> risks are quite vague so I'd stay on the performance side here for the
> default value (reuse). But probably based on my incomplete understanding of
> the lifecycles of HC objects, I'd feel better if we had such a "reset
> between iterations" switch.

I think we already do.

(In reply to Sebb from comment #7)
> (In reply to Rainer Jung from comment #6)
> > As far as I understand the situation reusing (the default) means e.g.
> > connections are kept open and are reused via HTTP Keep-Alive during the next
> > iteration. I do see this behavior using https, I expect it to happen for
> > http too. That's of course efficient but might not be what you want. A new
> > iteration typically is a new user who would start with new connections.
> >
> > Of course modelling the connection behavior in an exact way is beyond what
> > we support, but the decision to start a new iteration with new connections
> > could be interesting for some (approximate modeling). It is somehow
> > analogous to modeling the handshake rate by resetting the SSLContext,
> > although a new connection is of cours less disruptive than a new ssl
> > handshake. All in all not a critical feature.
> 
> Surelt that's already catered for by the "Use Keepalive" checkbox?
> If not checked, JMeter sends Connection: close.

I didn't mean to completely disable Keep-Alive but to not alow Keep-Alive from one itertaion to the next. The "Use Keepalive" checkbox would disable Keep-Alive completely.

> > In addition I think the optional behavior "reset instead of reuse" for http
> > and https is a good safety net against problems that might creep in due to
> > reuse of objects that might have more context than we expect (wrong basic
> > auth, wrong cookie, wrong client cert, wrong SNI etc.). Of course most of
> > these things do work now, but reusing the connections and the objects from
> > the HC libs as well as the SSLContext might introduce future bugs of the
> > sort "next iteration uses context data from the previous one" which could
> > then be worked around quickly by disabling the reuse. In general since the
> > risks are quite vague so I'd stay on the performance side here for the
> > default value (reuse). But probably based on my incomplete understanding of
> > the lifecycles of HC objects, I'd feel better if we had such a "reset
> > between iterations" switch.
>
> I think we already do.

I don't understand you. The current impl of reset (https.use.cached.ssl.context) is a NOP in the http case.

(In reply to Rainer Jung from comment #4)
> Yes, sorry, JMeter properties.
> 
> I think we don't need/want all three of them (individual control over
> connection reuse, HTTPClient reuse and SSLContext reuse). After some
> experimenting IMHO the useful cases are:
> 
> - full reuse of SSLContext, HTTPClient and existing connections
> - no reuse of SSLContext, HTTPClient and existing connections

Looks ok to me.
> 
> The default should be full reuse like it is today.
> 
> The question is though, whether the name "https.use.cached.ssl.context" for
> the property is good. Once could use a uniform switch to determine reuse in
> the HTTP case as well, something like reset.http.objects.before.iteration.
> Default would be "false" (no reset = reuse) and to start each iteration with
> fresh objects you would switch to "true".
> 
> The old flag could be deprecated. If a user would use both, the old and the
> new, we could warn during startup and the new flag would win.

Thanks for fix

(In reply to Rainer Jung from comment #8)
> (In reply to Sebb from comment #7)
> > (In reply to Rainer Jung from comment #6)
> > > As far as I understand the situation reusing (the default) means e.g.
> > > connections are kept open and are reused via HTTP Keep-Alive during the next
> > > iteration. I do see this behavior using https, I expect it to happen for
> > > http too. That's of course efficient but might not be what you want. A new
> > > iteration typically is a new user who would start with new connections.
> > >
> > > Of course modelling the connection behavior in an exact way is beyond what
> > > we support, but the decision to start a new iteration with new connections
> > > could be interesting for some (approximate modeling). It is somehow
> > > analogous to modeling the handshake rate by resetting the SSLContext,
> > > although a new connection is of cours less disruptive than a new ssl
> > > handshake. All in all not a critical feature.
> > 
> > Surelt that's already catered for by the "Use Keepalive" checkbox?
> > If not checked, JMeter sends Connection: close.
> 
> I didn't mean to completely disable Keep-Alive but to not alow Keep-Alive
> from one itertaion to the next. 

What do you mean by iteration?
If you mean the start of the next loop, one just needs to clear Use KeepAlive on the last sampler.

> The "Use Keepalive" checkbox would disable
> Keep-Alive completely.

Use Keepalive only applies to a single sample.
If deselected, it means the connection will be reset after it completes.

> 
> > > In addition I think the optional behavior "reset instead of reuse" for http
> > > and https is a good safety net against problems that might creep in due to
> > > reuse of objects that might have more context than we expect (wrong basic
> > > auth, wrong cookie, wrong client cert, wrong SNI etc.). Of course most of
> > > these things do work now, but reusing the connections and the objects from
> > > the HC libs as well as the SSLContext might introduce future bugs of the
> > > sort "next iteration uses context data from the previous one" which could
> > > then be worked around quickly by disabling the reuse. In general since the
> > > risks are quite vague so I'd stay on the performance side here for the
> > > default value (reuse). But probably based on my incomplete understanding of
> > > the lifecycles of HC objects, I'd feel better if we had such a "reset
> > > between iterations" switch.
> >
> > I think we already do.
> 
> I don't understand you. The current impl of reset
> (https.use.cached.ssl.context) is a NOP in the http case.

I meant that Use KeepAlive acts as the reset if deselected.

So I think we already have the means to do a reset each iteration.

(In reply to Sebb from comment #10)
> (In reply to Rainer Jung from comment #8)
> > (In reply to Sebb from comment #7)
> > > (In reply to Rainer Jung from comment #6)
> > > > As far as I understand the situation reusing (the default) means e.g.
> > > > connections are kept open and are reused via HTTP Keep-Alive during the next
> > > > iteration. I do see this behavior using https, I expect it to happen for
> > > > http too. That's of course efficient but might not be what you want. A new
> > > > iteration typically is a new user who would start with new connections.
> > > >
> > > > Of course modelling the connection behavior in an exact way is beyond what
> > > > we support, but the decision to start a new iteration with new connections
> > > > could be interesting for some (approximate modeling). It is somehow
> > > > analogous to modeling the handshake rate by resetting the SSLContext,
> > > > although a new connection is of cours less disruptive than a new ssl
> > > > handshake. All in all not a critical feature.
> > > 
> > > Surelt that's already catered for by the "Use Keepalive" checkbox?
> > > If not checked, JMeter sends Connection: close.
> > 
> > I didn't mean to completely disable Keep-Alive but to not alow Keep-Alive
> > from one itertaion to the next. 
> 
> What do you mean by iteration?

One loop for the same thread.

> If you mean the start of the next loop, one just needs to clear Use
> KeepAlive on the last sampler.

Depending on the logic controllers in the plan, there is no easy way to decide upon the last sampler, more precisely the last execution of the last sampler in one loop. It could change during plan execution. Most often it will be a defined logout sample or similar, but not necessarily. You e.g. might want to loop the last sampler until a condition becomes true.

> > The "Use Keepalive" checkbox would disable
> > Keep-Alive completely.
> 
> Use Keepalive only applies to a single sample.
> If deselected, it means the connection will be reset after it completes.

OK, you were talking about a single HTTP sampler, I was talking about the HTTP sampler default config element. See above for problem of defining the "last sampler".

> > > > In addition I think the optional behavior "reset instead of reuse" for http
> > > > and https is a good safety net against problems that might creep in due to
> > > > reuse of objects that might have more context than we expect (wrong basic
> > > > auth, wrong cookie, wrong client cert, wrong SNI etc.). Of course most of
> > > > these things do work now, but reusing the connections and the objects from
> > > > the HC libs as well as the SSLContext might introduce future bugs of the
> > > > sort "next iteration uses context data from the previous one" which could
> > > > then be worked around quickly by disabling the reuse. In general since the
> > > > risks are quite vague so I'd stay on the performance side here for the
> > > > default value (reuse). But probably based on my incomplete understanding of
> > > > the lifecycles of HC objects, I'd feel better if we had such a "reset
> > > > between iterations" switch.
> > >
> > > I think we already do.
> > 
> > I don't understand you. The current impl of reset
> > (https.use.cached.ssl.context) is a NOP in the http case.
> 
> I meant that Use KeepAlive acts as the reset if deselected.
> 
> So I think we already have the means to do a reset each iteration.

As I wrote, connection keep alive is only one of the arguments. General object reuse between loop iterations (HTTPClient instance) is what makes me feel more uncomfortable. If someone with enough HC knowledge than me comments strongly that such a reset will most likely not be necessary, I'm OK to forget about that reset feature for http.

Hi,
Can the patch be commited ?
Thanks

Hello,

I suggest we delay this bugfix to 3.1 unless there is a volunteer to test it and confirm it is OK (including Parallel Download)

Regards
Philippe

Hi,

well in jmeter 2.13 i had the following (so far not clear to me) issue with https where the https WAS mutual ssl.
I set up the ssl debug on and watched what happend with my http samplers.

I had a java key store whith 2 client certs.
I had a key store configuration element which starts at 0 and ends at 1 (2 elements).

Now i had one thread group and 2 http samplers both pointing to the same https server. One sampler was targeting https://server/uri and the second ../uri2.

Now i had the following problem.
The first http sampler picked the first ssl cert from my keystore [0] made a mutual ssl connection and execute .../uri. -great.

The second http sampler DOES NOT REUSE the connection and DOES NOT executed the ../uri2 BUT the second http sampler made a NEW connection (ssl) and picked a NEW client cert from the keystore [1]

It was STILL the SAME Thread just the next http sampler.
It was a mess because the user with the client cert on [1] was not allowed to execute the ../uri2. 

Then...i changed the parameter "https.use.cached.ssl.context" and put value "true".
I have no idea why...but it WORKED?!?. 
Means the first http sampler did mutual ssl and execute ../uri
The second http sampler did NOT the handshake again but execute the ../uri2 as the same user as the first sampler. perfect.
But why?

Still present in 4.0

Created attachment 35742 [details]
Amended Patch

Following dev mailing list discussion "Bug 58807" , this is a patch implementing what I suggested.

Created attachment 35743 [details]
Test plan with different cases

Created attachment 35744 [details]
NodeJS Project for client certificate authentication

NodeJS project that can be run :

node server.js

Author: pmouawad
Date: Sun Feb 25 21:19:42 2018
New Revision: 1825328

URL: http://svn.apache.org/viewvc?rev=1825328&view=rev
Log:
Bug 58807 - https.use.cached.ssl.context=false is broken
Based partly on Rainer Jung analysis and patch.
Bugzilla Id: 58807

Modified:
    jmeter/trunk/bin/jmeter.properties
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPHC4Impl.java
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPHCAbstractImpl.java
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerProxy.java
    jmeter/trunk/xdocs/changes.xml
    jmeter/trunk/xdocs/usermanual/properties_reference.xml

Author: pmouawad
Date: Tue Aug 28 21:12:36 2018
New Revision: 1839506

URL: http://svn.apache.org/viewvc?rev=1839506&view=rev
Log:
Bug 58807 - Reset SSL State on Thread Group iteration only (was https.use.cached.ssl.context=false is broken)
Document
Bugzilla Id: 58807

Modified:
    jmeter/trunk/xdocs/changes.xml
    jmeter/trunk/xdocs/usermanual/component_reference.xml
    
pmouawad@apache.org
	
11:13 PM (0 minutes ago)
	
to commits
Author: pmouawad
Date: Tue Aug 28 21:13:48 2018
New Revision: 1839507

URL: http://svn.apache.org/viewvc?rev=1839507&view=rev
Log:
Bug 58807 - Reset SSL State on Thread Group iteration only (was https.use.cached.ssl.context=false is broken)
Document
Bugzilla Id: 58807

Modified:
    jmeter/trunk/xdocs/usermanual/properties_reference.xml

*** Bug 62734 has been marked as a duplicate of this bug. ***

Hello all,

After migrating to Jmeter 5.1.1, I have the issue when I'm trying to send several requests with different client ssl certificates inside one tread group and one iteration. In Jmeter 4.0 I could set https.use.cached.ssl.context=false, but it's deprecated since Jmeter 5.0. I tried to close connection for the first request, but it didn't help me. Could you please help me how can I do it in Jmeter 5.0 ?

Thanks,
Vasili

This issue has been migrated to GitHub: https://github.com/apache/jmeter/issues/3759