search

Azure / azure-sdk-for-cpp

This repository is for active development of the Azure SDK for C++. For consumers of the SDK we recommend visiting our versioned developer docs at https://azure.github.io/azure-sdk-for-cpp.
MIT License
175 stars 126 forks source link

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

Open Slach opened 1 month ago

Slach commented 1 month ago

Query/Question I got some flaky errors inside my tests in clickhouse-server with azurite details here https://github.com/ClickHouse/ClickHouse/issues/60447 and here https://github.com/Azure/Azurite/pull/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 https://github.com/ClickHouse/ClickHouse/commit/d7fb851d172e2955ab81ea107ed58c0867a1929f 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[bot] commented 1 month ago

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

LarryOsterman commented 1 month ago

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 commented 1 month ago

ClickHouse used libCURL

Jinming-Hu commented 1 month ago

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 commented 1 month ago

@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 commented 1 month ago

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 commented 1 month ago

didn't find any handling of keep-alive header, I think it's just not supported. Look https://github.com/Azure/azure-sdk-for-cpp/blob/b4020493c4b0c2c1b5c81764be01b4ef0223bf4b/sdk/core/azure-core/src/http/curl/curl.cpp#L930-L985

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 commented 1 month ago

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

Slach commented 1 month ago

=) 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 commented 1 month ago

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.

gearama commented 1 month ago

I suspect there is a similar issue on winhttp

Slach commented 1 month ago

@RickWinter thank you kindly for adding issue to milestone

i asked in libcurl repo suggestions https://github.com/curl/curl/discussions/14487

Slach commented 1 month ago

maybe https://github.com/curl/curl/discussions/14487#discussioncomment-10302123 could help

LarryOsterman commented 1 month ago

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).