Bug 58099 - Performance : Lazily initialize HttpClient SSL Context to avoid its initialization even for HTTP only scenarios
Summary: Performance : Lazily initialize HttpClient SSL Context to avoid its initializ...
Status: RESOLVED FIXED
Alias: None
Product: JMeter - Now in Github
Classification: Unclassified
Component: HTTP (show other bugs)
Version: 2.6
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: JMeter issues mailing list
URL:
Keywords:
: 59020 (view as bug list)
Depends on:
Blocks: 52073
  Show dependency tree
 
Reported: 2015-07-04 13:12 UTC by Philippe Mouawad
Modified: 2016-02-19 01:08 UTC (History)
1 user (show)



Attachments
Patch fixing this issue (9.25 KB, patch)
2016-02-17 18:45 UTC, Philippe Mouawad
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Mouawad 2015-07-04 13:12:40 UTC
Currently HTTPClient 4 implementation eagerly creates an SSLContext , degrading in a important way performances of HttpClient init.

This is particularly annoying for tests using Concurrent download for resources and for test that do not use HTTPS at all.


This would be helpful as it would:
- improve overall performance particularly for Concurrent download
- Unblock https://issues.apache.org/bugzilla/show_bug.cgi?id=52073


This issue is created after closing Bug 57320.
Comment 1 Sebb 2015-08-29 16:17:06 UTC
Lazy init would work well for tests that don't use HTTPS.

However it would still cause a slight slowdown in tests that use HTTPS when the first HTTPS sample is performed. (The sample response time is not affected)

A work-round is to add a dummy HTTP request in a setUp thread group.

Note that there will be other initialisation overheads in JMeter.
Comment 2 Philippe Mouawad 2015-08-29 22:00:56 UTC
I don't think workaround works as the httpclient is shared in thread local but setup thread group threads die before thread group thread start.

The current state impacts uselessly tests that only run http, it drastically degrades performances of tests that use embedded resources parallel download.

For me this bug should be in our priorities.
Comment 3 Sebb 2015-08-29 23:08:04 UTC
The work-round seemed to work for me. At least some of the startup overhead was reduced. I think this is because of the static{} block.

What is the SSL code that is being invoked per thread?
I had a quick look and could not find anything SSL related apart from a call to JsseSSLManager sslMgr = (JsseSSLManager) SSLManager.getInstance() which is only done if the protocol is HTTPS.

But perhaps I have missed something.
Comment 4 Philippe Mouawad 2015-08-30 06:18:15 UTC
(In reply to Sebb from comment #3)
> The work-round seemed to work for me. At least some of the startup overhead
> was reduced. I think this is because of the static{} block.
> 
Read the thread on dev list called:

httpClient.getConnectionManager() performance with HTTP only
Oleg wrote this:

-----------------------------------
I see. For the time what you can do is to use a custom SSL socket
factory that lazily initializes SSL context when requested for the first
time. This is exactly what HC 3.1 does. It will be somewhat slower given
that one would need to mutex to synchronize access to the initialization
code

-----------------------------------




> What is the SSL code that is being invoked per thread?
> I had a quick look and could not find anything SSL related apart from a call
> to JsseSSLManager sslMgr = (JsseSSLManager) SSLManager.getInstance() which
> is only done if the protocol is HTTPS.
> 
> But perhaps I have missed something.
Comment 5 Philippe Mouawad 2016-02-17 18:45:50 UTC
Created attachment 33567 [details]
Patch fixing this issue
Comment 6 Philippe Mouawad 2016-02-17 18:46:02 UTC
*** Bug 59020 has been marked as a duplicate of this bug. ***
Comment 7 Sebb 2016-02-17 21:21:30 UTC
Comment on attachment 33567 [details]
Patch fixing this issue

[New file needs an AL header]

Looks like adaptee is created using double-checked locking.
That is not thread-safe unless protected by volatile (or a full lock, in which case no point in double-checking).

Given that the adaptee depends only on the value of CPS_HTTPS, and that is only checked at startup, there will only ever be one adaptee. So the code could perhaps use the IODH idiom instead.
Comment 8 Philippe Mouawad 2016-02-17 21:31:50 UTC
Author: pmouawad
Date: Wed Feb 17 21:31:24 2016
New Revision: 1730947

URL: http://svn.apache.org/viewvc?rev=1730947&view=rev
Log:
Bug 58099 - Performance : Lazily initialize HttpClient SSL Context to avoid its initialization even for HTTP only scenarios
Bugzilla Id: 58099

Added:
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/LazySchemeSocketFactory.java   (with props)
Modified:
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPHC4Impl.java
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/util/SlowHC4SSLSocketFactory.java
    jmeter/trunk/xdocs/changes.xml
Comment 9 Philippe Mouawad 2016-02-17 21:36:46 UTC
(In reply to Sebb from comment #7)
> Comment on attachment 33567 [details]
> Patch fixing this issue
> 
> [New file needs an AL header]
fixed
> 
> Looks like adaptee is created using double-checked locking.
> That is not thread-safe unless protected by volatile (or a full lock, in
> which case no point in double-checking).
> 
Thanks, used volatile.

> Given that the adaptee depends only on the value of CPS_HTTPS, and that is
> only checked at startup, there will only ever be one adaptee. So the code
> could perhaps use the IODH idiom instead.
Didn't do it, feel free to do it if you think it is useful.
Comment 10 Sebb 2016-02-17 23:48:03 UTC
When I wrote:

"there will only ever be one adaptee"

I meant that for a given test run, there will only be a single adaptee.

However this may be either a HC4TrustAllSSLSocketFactory or a SlowHC4SSLSocketFactory

The commit that was applied no longer uses SlowHC4SSLSocketFactory, however AFAICT it should (as per the diff that was originally posted)
Comment 11 Philippe Mouawad 2016-02-18 09:07:45 UTC
Hi,
No SlowHC4SSLSocketFactory is not useful anymore, look at code it just extends HC4TrustAllSSLSocketFactory without anything specific.
I deprecated it , we could even remove it.
Comment 12 Sebb 2016-02-18 10:29:04 UTC
(In reply to Philippe Mouawad from comment #11)
> Hi,
> No SlowHC4SSLSocketFactory is not useful anymore, look at code it just
> extends HC4TrustAllSSLSocketFactory without anything specific.

That's not so; it passes cps to HttpSSLProtocolSocketFactory.
If that's not working it needs to be fixed, not arbitrarily abandoned.

> I deprecated it , we could even remove it.

In any case, this issue is entirely separate.
Removal of SlowHC4SSLSocketFactory would require a separate issue (but I don't think it should be removed).

Please revert the parts that removed SlowHC4SSLSocketFactory.
Comment 13 Philippe Mouawad 2016-02-18 10:45:42 UTC
Hi Sebb,
(In reply to Sebb from comment #12)
> (In reply to Philippe Mouawad from comment #11)
> > Hi,
> > No SlowHC4SSLSocketFactory is not useful anymore, look at code it just
> > extends HC4TrustAllSSLSocketFactory without anything specific.
> 
> That's not so; it passes cps to HttpSSLProtocolSocketFactory.
> If that's not working it needs to be fixed, not arbitrarily abandoned.
> 

I am aware of that,look at HC4TrustAllSSLSocketFactory, it takes into account CPS also.

> > I deprecated it , we could even remove it.
> 
> In any case, this issue is entirely separate.

> Removal of SlowHC4SSLSocketFactory would require a separate issue (but I
> don't think it should be removed).

No it is not needed as HttpSSLProtocolSocketFactory does its job already
> 
> Please revert the parts that removed SlowHC4SSLSocketFactory.

It is related as this removal occured through implementation of  LazySchemeSocketFactory.

Unless I am missing something, I don't see any issue here
Comment 14 Philippe Mouawad 2016-02-18 23:45:10 UTC
Author: pmouawad
Date: Thu Feb 18 23:44:53 2016
New Revision: 1731172

URL: http://svn.apache.org/viewvc?rev=1731172&view=rev
Log:
Bug 58099 - Performance : Lazily initialize HttpClient SSL Context to avoid its initialization even for HTTP only scenarios
Fix regression on HTTPS tests, No route to host...
Bugzilla Id: 58099

Modified:
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/LazySchemeSocketFactory.java
Comment 15 Sebb 2016-02-19 00:13:33 UTC
(In reply to Philippe Mouawad from comment #13)
> Hi Sebb,
> (In reply to Sebb from comment #12)
> > (In reply to Philippe Mouawad from comment #11)
> > > Hi,
> > > No SlowHC4SSLSocketFactory is not useful anymore, look at code it just
> > > extends HC4TrustAllSSLSocketFactory without anything specific.
> > 
> > That's not so; it passes cps to HttpSSLProtocolSocketFactory.
> > If that's not working it needs to be fixed, not arbitrarily abandoned.
> > 
> 
> I am aware of that,look at HC4TrustAllSSLSocketFactory, it takes into
> account CPS also.
> 
> > > I deprecated it , we could even remove it.
> > 
> > In any case, this issue is entirely separate.
> 
> > Removal of SlowHC4SSLSocketFactory would require a separate issue (but I
> > don't think it should be removed).
> 
> No it is not needed as HttpSSLProtocolSocketFactory does its job already
> > 
> > Please revert the parts that removed SlowHC4SSLSocketFactory.
> 
> It is related as this removal occured through implementation of 
> LazySchemeSocketFactory.
> 
> Unless I am missing something, I don't see any issue here

It does seem to work OK.

It seems SlowHC4SSLSocketFactory was effectively made obsolete in Bugzilla 55455.

So it's not suprising that dropping it as a part of this bug is confusing.
Comment 16 Sebb 2016-02-19 01:08:10 UTC
URL: http://svn.apache.org/viewvc?rev=1731180&view=rev
Log:
Performance : Lazily initialize HttpClient SSL Context to avoid its initialization even for HTTP only scenarios
Simplify by using IODH idiom
Bugzilla Id: 58099

Modified:
    jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/LazySchemeSocketFactory.java
Comment 17 The ASF infrastructure team 2022-09-24 20:38:00 UTC
This issue has been migrated to GitHub: https://github.com/apache/jmeter/issues/3620