swagger-api / swagger-codegen

swagger-codegen contains a template-driven engine to generate documentation, API clients and server stubs in different languages by parsing your OpenAPI / Swagger definition.
http://swagger.io
Apache License 2.0
16.87k stars 6.02k forks source link

no connection pooling in ApiClient generated for cpprest #10233

Open ilanguev opened 4 years ago

ilanguev commented 4 years ago
Description

HTTP connections are not getting re-used across multiple calls to REST APIs on the same baseUrl

Swagger-codegen version

I am using swagge-codegen 2.4.13.

I don't think it is a regression, see below.

Swagger declaration file content or url

I am using the PetStore REST API: https://petstore.swagger.io/v2/swagger.json

Command line used for generation

swagger-codegen generate -i swagger.json -l cpprest -o ./cpprest-client

Steps to reproduce

I have a simplistic multi-threaded client that makes 100,000 requests to "GET ... store/inventory". I will post the source code for the client down below for reference. While the client is running, I am monitoring outgoing TCP connections with "netstat -anp|grep ". The list of local port numbers connected to the server keeps changing, indicating lack of connection pooling.

Related issues/PRs

N/A

Suggest a fix/enhancement

Looking at the template for generating ApiClient.cpp: https://github.com/swagger-api/swagger-codegen/blob/3089fd3856654e99d7bd0a51c8764080c2868f46/modules/swagger-codegen/src/main/resources/cpprest/apiclient-source.mustache#L96

... it creates a new instance of web::http::client::http_client client on each ApiClient::callApi invocation.

I believe the connection pooling logic is encapsulated inside http_client/http_client_asio. As far as I can tell, limiting the scope of the "client" lifetime to callApi invocation is what causing connection pooling not working.

In fact, if I move http_client creation to the constructor of ApiClient and then re-use it for all calls to callApi, I see the connections properly pooled:

ApiClient::ApiClient(std::shared_ptr<ApiConfiguration> configuration )
    : m_Configuration(configuration)
{
    m_client = std::make_shared<web::http::client::http_client>(m_Configuration->getBaseUrl(), m_Configuration->getHttpConfig());
}
...
pplx::task<web::http::http_response> ApiClient::callApi(
...
    return m_client->request(request);
}

This is not meant as a production-worthy fix, i.e. there is no check for configuration == NULL, etc.

But perhaps I am missing something in how connection pooling is supposed to work with cpprest? It seems that avoiding constant TCP reconnects and ssl handshake is something anyone using swagger-codegen would expect, perhaps I am just not doing it right?

Multi-threaded client I used for testing
#include <thread>
#include <chrono>
#include <vector>

#include "api/StoreApi.h"
using namespace io::swagger::client::api;

#define NUM_THREADS 100
#define NUM_REQUESTS 100000
#define SLEEP_SECONDS 0    

void getInventory(std::shared_ptr<StoreApi> api, int i)
{
    api->getInventory().then([=](pplx::task<std::map<utility::string_t, int32_t>> inventory) {
        try {
            for (auto it: inventory.get()) { 
                std::cout << "[" << std::this_thread::get_id() << "]" << i << ":" << 
                        it.first << "=>" << it.second << std::endl; 
            }
        } catch(const std::exception& e) {
            std::cout << "getInventory() exception: " << e.what() << '\n';
        }
    }).wait();
}

void getInventoryManyTimes(std::shared_ptr<StoreApi> api)
{
    for (auto i = 0; i < NUM_REQUESTS; ++i) {
        getInventory(api, i);
        std::this_thread::sleep_for (std::chrono::seconds(SLEEP_SECONDS));
    }
}

int main()
{
    std::shared_ptr<ApiConfiguration> apiConfig(new ApiConfiguration);
    apiConfig->setBaseUrl("https://virtserver.swaggerhub.com/xxx/MyPetstore/1.0.0");
    std::shared_ptr<ApiClient> apiClient(new ApiClient(apiConfig));

    std::shared_ptr<StoreApi> api(new StoreApi(apiClient));

    std::vector<std::thread> threads;
    for (auto i = 0; i < NUM_THREADS; ++i) {
        std::thread t(getInventoryManyTimes, api);
        threads.push_back(std::move(t));
    }

    for  (auto &t: threads) {
        t.join();
    }
}
ilanguev commented 4 years ago

Any comment on this?