Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Could you suggest to code snippet how to properly initialize Azure cpp SDK for respect keep-alive server header? #5877

Open
Slach opened this issue Aug 6, 2024 · 14 comments · May be fixed by #5930
Assignees
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that
Milestone

Comments

@Slach
Copy link

Slach commented Aug 6, 2024

Query/Question
I got some flaky errors inside my tests in clickhouse-server with azurite
details here
ClickHouse/ClickHouse#60447 and here Azure/Azurite#2443

clickhouse-server use aws-cpp-sdk to make requests to azurite
but after receive from azurite

Connection: keep-alive
Keep-Alive: timeout=5

and FIN TCP packet
aws-sdk/clickhouse-server doesn't send FIN ACK and trying to write to already closed connection
unfortunatelly in ClickHouse/ClickHouse@d7fb851
clickhouse team decided just retry on any transport error

Why is this not a Bug or a feature Request?

Could you suggest to code snippet how to properly initialize Azure cpp SDK for respect keep-alive server header?

@github-actions github-actions bot added Azure.Core Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Aug 6, 2024
Copy link

github-actions bot commented Aug 6, 2024

Thank you for your feedback. Tagging and routing to the team member best able to assist.

@LarryOsterman
Copy link
Member

QQ: The Azure SDK supports two separate transports - one for WinHTTP, the other for libCURL. Which transport are you using?

WinHTTP should support the keep-alive header properly, but with a quick look at the code, the value of the "keep-alive" header does not appear to be respected, only that the presence of the header.

@Slach
Copy link
Author

Slach commented Aug 7, 2024

ClickHouse used libCURL

@Jinming-Hu
Copy link
Member

How is this related to Azure SDK?
I think you should open issue either in https://github.com/aws/aws-sdk-cpp repo or https://github.com/Azure/Azurite repo.

@Slach
Copy link
Author

Slach commented Aug 8, 2024

@Jinming-Hu it definitely related to Azure SDK

ClickHouse uses Azure SDK, under non windows OS
Azure SDK use libCURL, may i ask you again code snippet

how to initialize Azure SDK properly, to respect server side header Keep-alive: timeout=5 which sente from azurite?

@Jinming-Hu
Copy link
Member

We implement HTTP protocol on top of libcurl raw TCP connections. Looking through https://github.com/Azure/azure-sdk-for-cpp/blob/b4020493c4b0c2c1b5c81764be01b4ef0223bf4b/sdk/core/azure-core/src/http/curl/curl.cpp, I didn't find any handling of keep-alive header, I think it's just not supported.

@Slach
Copy link
Author

Slach commented Aug 8, 2024

didn't find any handling of keep-alive header, I think it's just not supported.
Look

if (non2xxAfter100ContinueWithNonzeroContentLength)
{
m_httpKeepAlive = false;
}
else
{
// HTTP <=1.0 is "close" by default. HTTP 1.1 is "keep-alive" by default.
// The value can also be "keep-alive, close" (i.e. "both are fine"), in which case we are
// preferring to treat it as keep-alive.
// (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Connection)
// Should it come to HTTP/2 and HTTP/3, they are "keep-alive", but any response from HTTP/2 or
// /3 containing a "Connection" header should be considered malformed.
// (HTTP/2: https://httpwg.org/specs/rfc9113.html#ConnectionSpecific
// HTTP/3: https://httpwg.org/specs/rfc9114.html#rfc.section.4.2)
//
// HTTP/2+ are supposed to create persistent ("keep-alive") connections per host,
// and close them by inactivity timeout. Given that we don't have such mechanism implemented
// at this moment, we are closing all non-1.x connections immediately, which, in the worst case,
// would only mean there's a perf hit, but the communication flow is expected to be correct.
if (m_response->GetMajorVersion() == 1)
{
std::string connectionHeaderValue;
{
const Core::CaseInsensitiveMap& responseHeaders = m_response->GetHeaders();
const auto connectionHeader = responseHeaders.find("Connection");
if (connectionHeader != responseHeaders.cend())
{
connectionHeaderValue
= Core::_internal::StringExtensions::ToLower(connectionHeader->second);
}
}
const bool hasConnectionKeepAlive
= connectionHeaderValue.find("keep-alive") != std::string::npos;
if (m_response->GetMinorVersion() >= 1)
{
// HTTP/1.1+
const bool hasConnectionClose = connectionHeaderValue.find("close") != std::string::npos;
m_httpKeepAlive = (!hasConnectionClose || hasConnectionKeepAlive);
}
else
{
// HTTP/1.0
m_httpKeepAlive = hasConnectionKeepAlive;
}
}
else
{
// We don't expect HTTP/0.9, 2.0 or 3.0 in responses.
// Barring rejecting as malformed, the safest thing to do here is to assume the connection is
// not reusable.
m_httpKeepAlive = false;
}
}

it should be supported

I just ask how to properly initialize SDK to handle standard HTTP behavior.

I believe, SDK shall not try to write to socket which already closed from server side and don't need to make useless packet send.

@Jinming-Hu
Copy link
Member

@Slach the code snippet you quoted handles Connection: keep-alive. It doesn't handle keep-alive: xxxx

@Slach
Copy link
Author

Slach commented Aug 8, 2024

=) ok. got it, thanks for suggestion

But what about to add properly handle connection closing?
Let's convert this issue from question to bug-report?

When server sent Keep-Alive: timeout=5 and close connection after 5 second, then Azure CPP SDK will fire parasite traffic and fire execption when get TCP RST from server after try to write already closed connection

@LarryOsterman
Copy link
Member

The Connection: Close header should be properly handled on all transports. What I believe is not correctly handled on curl is keep-alive: timeout. If the keep-alive header is present, the curl transport treats it the same as connection: keep-alive.

@RickWinter RickWinter added this to the 2024-10 milestone Aug 9, 2024
@gearama
Copy link
Member

gearama commented Aug 9, 2024

I suspect there is a similar issue on winhttp

@Slach
Copy link
Author

Slach commented Aug 11, 2024

@RickWinter thank you kindly for adding issue to milestone

i asked in libcurl repo suggestions
curl/curl#14487

@Slach
Copy link
Author

Slach commented Aug 11, 2024

maybe curl/curl#14487 (comment) could help

@LarryOsterman
Copy link
Member

I think I was unclear: For performance reasons, the Azure SDK for C++'s libcurl transport does not use the libcurl implementation of HTTP, it only uses libcurl for connection management. All HTTP interactions are managed by the libcurl transport directly, as is all the connection pooling behavior.

I suspect this may be a bug in the Azure SDK for C++'s libCURL transport implementation which does not appear respect the parameters to the keep-alive HTTP header (I have not done any investigation on this issue however).

@gearama gearama linked a pull request Aug 30, 2024 that will close this issue
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Core Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team question The issue doesn't require a change to the product in order to be resolved. Most issues start as that
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants