XmlApiClient is not thread safe

[msiuts]

The XmlApiClient is not threadsafe because the constructor calls this super-constructor:

protected ApiClient(ApiCommandProcessor<REQUEST> commandProcessor, ApiSession session) {
        this(commandProcessor, new HttpClient(), session);
    }

The problem here is, that new HttpClient() constructs a HttpClient with a SimpleHttpConnectionManager and this can cause an Exception and this log message:

  WARN org.apache.commons.httpclient.SimpleHttpConnectionManager  - SimpleHttpConnectionManager being used incorrectly.  Be sure that HttpMethod.releaseConnection() is always called and that only one thread and/or method is using this connection manager at a time.

In my opinion the HttpClient which is used should be injected and build and configured elsewhere. Or it should be constructed with a MultiThreadedHttpConnectionManager.

But maybe the whole ApiClient is not meant to be threadsafe - that's why I'd like to discuss this issue before implementing a fix. What do you think?

[msales-silverpop]

Moritz,

Your proposal regarding the construction of HttpClient with the MultiThreadedHttpConnectionManager makes sense. There is allowance for injection of the HttpClient in the other constructor, but your suggestion improves the default behavior. Nice work!

Thanks,

-Matt

---

Matt Sales
Manager, Engineering | Silverpophttp://www.silverpop.com/
678-742-0105

200 Galleria Parkway, Suite 1000
Atlanta GA 30339
Email.Marketing.Automation.

From: Moritz Siuts <notifications@github.commailto:notifications@github.com>
Reply-To: Silverpop/engage-api-client <reply@reply.github.commailto:reply@reply.github.com>
Date: Wednesday, May 7, 2014 at 9:54 AM
To: Silverpop/engage-api-client <engage-api-client@noreply.github.commailto:engage-api-client@noreply.github.com>
Subject: [engage-api-client] XmlApiClient is not thread safe (#16)

The XmlApiClient is not threadsafe because the constructor calls this super-constructor:

protected ApiClient(ApiCommandProcessor commandProcessor, ApiSession session) {
this(commandProcessor, new HttpClient(), session);
}

The problem here is, that new HttpClient() constructs a HttpClient with a SimpleHttpConnectionManager and this can cause an Exception and this log message:

WARN org.apache.commons.httpclient.SimpleHttpConnectionManager - SimpleHttpConnectionManager being used incorrectly. Be sure that HttpMethod.releaseConnection() is always called and that only one thread and/or method is using this connection manager at a time.

In my opinion the HttpClient which is used should be injected and build and configured elsewhere. Or it should be constructed with a MultiThreadedHttpConnectionManager.

But maybe the whole ApiClient is not meant to be threadsafe - that's why I'd like to discuss this issue before implementing a fix. What do you think?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/16.

_CONFIDENTIALITY NOTICE and DISCLAIMER_
This message and any attachment are confidential and may be
privileged or otherwise protected from disclosure and solely for
the use of the person(s) or entity to whom it is intended. If you
have received this message in error and are not the intended
recipient, please notify the sender immediately and delete this
message and any attachment from your system. If you are not the
intended recipient, be advised that any use of this message is
prohibited and may be unlawful, and you must not copy this
message or attachment or disclose the contents to any other person.

[msiuts]

Ok I'm going to use MultiThreadedHttpConnectionManager.

For the injection:
Actually it's not possible to inject a custom httpclient. You are right that the other constructor has parameter for the httpclient, but ApiClient is an abstract class.

The XmlApiClient which is actually used in client code does not have a parameter for httpclient in any of it's constructors.

I think one should add those constructors too, because there is a lot of configuration possible for a httpclient instance.