SNOW-1737940: Support keepalive parameters in LibSfClient by sfc-gh-ext-simba-jy · Pull Request #841 · snowflakedb/libsnowflakeclient

sfc-gh-ext-simba-jy · 2025-03-08T01:33:43Z

No description provided.

…flakeclient into SNOW-1737940

cpp/lib/HeartbeatBackground.cpp

…rning

cpp/lib/HeartbeatBackground.hpp

cpp/lib/HeartbeatBackground.cpp

lib/client.c

cpp/lib/HeartbeatBackground.cpp

lib/client.c

sfc-gh-pbulawa · 2026-02-24T06:55:09Z

cpp/lib/HeartbeatBackground.cpp

+        if (!renew_session(curl, sf, err))
+        {
+            CXX_LOG_TRACE("sf::HeartbeatBackrgound::renew_session_sync::Failed to renew session");
+            stop_heart_beat_for_this_session(sf);


There's a deadlock here as the m_lock is used in the renewSession and then the same lock is used in the underlying removeConnection method.

This could be potentially fixed by the usage of RecursiveMutex instead of the MutexUnique

Since these two methods don’t call each other and are handled by separate threads, I’m trying to understand the reasoning behind the deadlock concern. Could you clarify why you think a deadlock might happen here?

There is a call chain on the same thread:

HeartbeatBackground::renewSession() acquires MutexUnique guard(m_lock)

While holding m_lock, it calls renew_session_sync(itr->second)

renew_session_sync(), if renew_session() fails, calls stop_heart_beat_for_this_session(sf)

stop_heart_beat_for_this_session() calls bg.removeConnection(sf)

removeConnection() tries to acquire MutexGuard m_guard(m_lock) - deadlock

Hello @sfc-gh-pbulawa, thank you for your feedback. After investigating the issue, I confirmed that this workflow can indeed cause a deadlock. Upon comparing the code flow with the ODBC implementation, I realized that stop_heartbeat is not required in renew_session_sync. Therefore, I have removed the stop_heartbeat_for_this_session call from renew_session_sync, which should resolve this issue.

sfc-gh-pbulawa · 2026-02-24T06:59:51Z

cpp/lib/HeartbeatBackground.cpp

+        }
+    }
+
+    heartbeatReq HeartbeatBackground::genHeartBeatReq(SF_CONNECT* connection)


Let's acquire the sf->mutex_tokens here, as the Connection parameters read is unprotected right now, which could cause a race condition, e.g., when the token is updated during the query execution.

I’m not entirely sure which tokens you’re referring to. The heartbeat request doesn’t include any body content, and it only needs the protocol, host, and port to construct the URL. You can find the POST request here: https://github.com/snowflakedb/libsnowflakeclient/pull/841/changes#diff-c11feb72994cdbbcf8f21da1a069803f0abbf5073e068b4ec4f793b5cef71791R176

genHeartBeatReq calls create_header:

libsnowflakeclient/cpp/lib/HeartbeatBackground.cpp

Line 291 in 3d1974b

if (!create_header(connection, httpExtraHeaders, &connection->error)) {

And create_header in connection.c does read the token without any lock:

libsnowflakeclient/lib/connection.c

Lines 182 to 188 in 3d1974b

sf_bool STDCALL create_header(SF_CONNECT *sf, SF_HEADER *header, SF_ERROR_STRUCT *error) {

sf_bool ret = SF_BOOLEAN_FALSE;

size_t header_token_size;

size_t header_direct_query_token_size;

size_t header_service_name_size;

const char *token = header->renew_session ? sf->master_token : sf->token;

It reads sf->token (or sf->master_token), which is exactly the kind of field protected by sf->mutex_tokens. Another thread performing query execution or session renewal can update these tokens concurrently.

Thank you for the clarification @sfc-gh-pbulawa. I applied to this. Thank you!

In this case, I believe the ODBC may need to apply this logic. WDYT?

Yes, if you could update it in odbc that would be great.

cpp/lib/HeartbeatBackground.cpp

sfc-gh-pbulawa

PTAL at the comments

…flakeclient into SNOW-1737940

sfc-gh-pbulawa · 2026-03-16T15:35:31Z

cpp/lib/HeartbeatBackground.cpp

+
+    void start_heart_beat_for_this_session(SF_CONNECT* sf)
+    {
+        _mutex_lock(&sf->mutex_heart_beat);


Lock ordering risks deadlock start_heart_beat_for_this_session and stop_heart_beat_for_this_session lock sf->mutex_heart_beat then call into HeartbeatBackground which acquires m_lock. However, renewSession iterates failedConnection under m_lock and then acquires conn->mutex_heart_beat. If these two orderings ever overlap, a deadlock occurs. Lock ordering must be documented and enforced (e.g., always acquire m_lock before mutex_heart_beat).

sfc-gh-pbulawa · 2026-03-16T15:39:39Z

cpp/lib/HeartbeatBackground.cpp

+        // join
+        if (m_worker != NULL)
+        {
+            m_worker->join();


Exception can escape. m_worker->join() can throw std::system_error. If it does, the exception propagates out of the destructor, causing std::terminate. Let's wrap in try/catch and log.

sfc-gh-pbulawa · 2026-03-16T15:40:32Z

tests/test_heartbeat.cpp

+    test_heartbeat(sf);
+
+    assert_false(sf_strncasecmp(previous_sessiontoken, sf->token, strlen(sf->token)) == 0);
+    assert_false(sf_strncasecmp(previous_masterToken, sf->master_token, strlen(sf->token)) == 0);


Suggested change

assert_false(sf_strncasecmp(previous_masterToken, sf->master_token, strlen(sf->token)) == 0);

assert_false(sf_strncasecmp(previous_masterToken, sf->master_token, strlen(sf->master_token)) == 0);

sfc-gh-pbulawa · 2026-03-16T15:41:18Z

cpp/lib/Mutex.cpp

+    void create_recursive_mutex(void** mutex, uint64_t id)
+    {
+        try {
+            *mutex = (void*) new Snowflake::Client::RecursiveMutex(id);


What if creation fails? The mutex will never be set.

sfc-gh-pbulawa · 2026-03-16T15:41:37Z

cpp/lib/Mutex.cpp

+    void free_recursive_mutex(void** mutex)
+    {
+        try {
+            delete static_cast<Snowflake::Client::RecursiveMutex*>(*mutex);


We should have a null check here for safety.

Copilot

Pull request overview

Adds client-session keepalive support to LibSfClient by sending keepalive session parameters during login and running a background heartbeat/renewal loop when enabled.

Changes:

Introduces new connection attributes and session parameters: CLIENT_SESSION_KEEP_ALIVE and CLIENT_SESSION_KEEP_ALIVE_HEARTBEAT_FREQUENCY.
Adds a C++ heartbeat background worker (plus C-callable wrappers) and recursive mutex plumbing to renew tokens safely.
Adds/updates tests and mock expected JSON to cover the new session parameters and heartbeat behavior.

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
tests/test_heartbeat.cpp	New heartbeat/keepalive integration tests (threaded + manual).
tests/mock/test_mock_mfa_token_caching.c	Forces keepalive off in MFA mock tests to match new request payload.
tests/mock/test_data/expected_second_mfa_request.json	Updates expected request JSON to include keepalive session parameters.
tests/mock/test_data/expected_first_mfa_request.json	Updates expected request JSON to include keepalive session parameters.
tests/CMakeLists.txt	Adds `test_heartbeat` and optional `HEARTBEAT_DEBUG` compile definitions.
lib/mutex.h	C API for creating/freeing a recursive mutex implemented in C++.
lib/heart_beat_background.h	C API for starting/stopping heartbeat and synchronously renewing session.
lib/connection.c	Adds keepalive session parameters to the login request body.
lib/client_int.h	Adds heartbeat URL/constants and extends error code constants; wraps uuid APIs for C++.
lib/client.c	Stores keepalive settings, session ID, starts/stops heartbeat on connect/term, and adds new set/get attributes.
include/snowflake/client.h	Exposes new attributes and adds heartbeat-related fields to `SF_CONNECT`.
include/snowflake/SFURL.hpp	Adds a new SFURL constructor and reorders some members.
cpp/util/SnowflakeCppUtil.cpp	Adjusts cJSON→picojson conversion to avoid leaking the printed JSON string.
cpp/util/SFURL.cpp	Implements the new SFURL constructor.
cpp/lib/SnowflakeUtil.cpp	Removes old duplicate C wrapper utilities (moved to cpp/util).
cpp/lib/Mutex.hpp	Adds C++ Mutex/RecursiveMutex wrappers (used by heartbeat + C bridge).
cpp/lib/Mutex.cpp	Implements recursive mutex bridge functions and mutex wrappers.
cpp/lib/IAuth.cpp	Uses URL constants and new SFURL constructor when building authenticator URL.
cpp/lib/HeartbeatBackground.hpp	Declares the singleton heartbeat worker and heartbeat request snapshot struct.
cpp/lib/HeartbeatBackground.cpp	Implements heartbeat worker thread, heartbeat POSTs, and session renew logic + C wrappers.
CMakeLists.txt	Adds HEARTBEAT option and includes new heartbeat/mutex sources/headers in builds.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

cpp/lib/HeartbeatBackground.cpp

+            MutexGuard m_guard(m_lock);
+
+            m_connections[connection->session_id] = connection;
+
+            if (m_worker == NULL)


cpp/lib/HeartbeatBackground.cpp

+            cJSON* resp_data = NULL;
+            SF_ERROR_STRUCT* err = NULL;
+
+            CXX_LOG_TRACE("sf::HeartbeatBackground::sendQueuedHeartBeatReq::sending heartbeat for session %s", conn.sessionId.c_str());
+
+            if (http_perform(curl, POST_REQUEST_TYPE, (char*)destination.c_str(), httpExtraHeaders, NULL, NULL, &resp_data,
+                NULL, NULL, conn.retryTimeout, conn.networkTimeout, SF_BOOLEAN_FALSE, err,
+                conn.isInsecuremode, conn.isOcspOpen, conn.crlCheck, conn.crlAdvisory, conn.crlAllowNoCrl,
+                conn.crlDiskCaching, conn.crlMemoryCaching, conn.crlDownloadTimeout,
+                conn.retryCurlCount, 0, conn.maxRetryCount, &elapsedTime, &retried_count, NULL, SF_BOOLEAN_FALSE,
+                proxy, noProxy, SF_BOOLEAN_FALSE, SF_BOOLEAN_FALSE))


tests/test_heartbeat.cpp

+#ifndef HEARTBEAT_DEBUG
+    return;
+#endif // 
+
+    SF_UNUSED(unused);


tests/test_heartbeat.cpp

+    sf_sleep_ms(30 * 1000);
+    assert_false(sf_strncasecmp(previous_sessiontoken, sf->token, strlen(sf->token)) == 0);
+    assert_false(sf_strncasecmp(previous_masterToken, sf->master_token, strlen(sf->token)) == 0);
+


tests/test_heartbeat.cpp

+    test_heartbeat(sf);
+
+    assert_false(sf_strncasecmp(previous_sessiontoken, sf->token, strlen(sf->token)) == 0);
+    assert_false(sf_strncasecmp(previous_masterToken, sf->master_token, strlen(sf->token)) == 0);


cpp/lib/Mutex.cpp

+
+        // trace to get the correlate OS reference with our internal mutex,
+        CXX_LOG_TRACE("sf::RecursiveMutex,/%s mutex=%lu", mutexName, m_id);
+    }


tests/test_heartbeat.cpp

+    sf_bool client_session_keep_alive = SF_BOOLEAN_TRUE;
+    snowflake_set_attribute(sf, SF_CON_CLIENT_SESSION_KEEP_ALIVE, &client_session_keep_alive);
+    snowflake_connect(sf);
+    std::this_thread::sleep_for(std::chrono::milliseconds(100));
+    //Make sure renew_session_sync work well
+    *result = renew_session_sync(sf);
+


lib/client.c

+            if (sf->client_session_keep_alive)
+            {
+                start_heart_beat_for_this_session(sf);
+            }
+            else
+            {
+                stop_heart_beat_for_this_session(sf);


cpp/util/SnowflakeCppUtil.cpp

+            std::string dataStr = std::string(jsonStr);
 			strToPicoJson(picojson, dataStr);
+			SF_FREE(jsonStr);


include/snowflake/SFURL.hpp

   */
  SFURL(const SFURL &copy);

+  SFURL(std::string& protocol, std::string& host, std::string& port);


sfc-gh-ext-simba-jy and others added 25 commits March 7, 2025 17:32

implement heartbeat

8a557a5

Merge branch 'master' into SNOW-1737940

dbdc2d3

add vector

7134b12

Merge branch 'SNOW-1737940' of https://github.com/snowflakedb/libsnow…

9f3bfb0

…flakeclient into SNOW-1737940

fix

4a40c8c

fix

61cf865

fix

3a12500

remove unused

2a94fde

fix

71b2a2a

add testings

d49de00

add thread

e333406

fix

d0522e7

Merge branch 'master' into SNOW-1737940

3d9aa0f

fix

9c5fd2c

Merge branch 'SNOW-1737940' of https://github.com/snowflakedb/libsnow…

ea14534

…flakeclient into SNOW-1737940

add test

b35d473

fix

75492d4

fix

f30f449

testing

2da3bbe

fix

c73e07e

remove header comments

c8a4485

refactored testing

45fa014

fix token request

959667a

fix mac and linux issue

1cef766

remove white spaces

c6fd47c

sfc-gh-ext-simba-jy requested a review from sfc-gh-ext-simba-hx March 12, 2025 21:45

Merge branch 'master' into SNOW-1737940

56708d7

sfc-gh-ext-simba-hx reviewed Mar 13, 2025

View reviewed changes

cpp/lib/HeartbeatBackground.cpp Outdated Show resolved Hide resolved

sfc-gh-ext-simba-hx reviewed Mar 13, 2025

View reviewed changes

cpp/lib/HeartbeatBackground.cpp Outdated Show resolved Hide resolved

sfc-gh-ext-simba-hx reviewed Mar 13, 2025

View reviewed changes

cpp/lib/HeartbeatBackground.cpp Show resolved Hide resolved

replace headers in heatbeat q with shared_ptr and fix code quality wa…

f7fe204

…rning

sfc-gh-ext-simba-jy requested a review from sfc-gh-ddas January 21, 2026 22:21

sfc-gh-ext-simba-jy and others added 2 commits January 21, 2026 14:33

fix warning

7e95032

Merge branch 'master' into SNOW-1737940

6afed0c

sfc-gh-ddas requested changes Feb 13, 2026

View reviewed changes

sfc-gh-ext-simba-jy added 4 commits February 16, 2026 06:05

updated the PR based on the comments

97c0a50

fix

4f836e2

fix

afb70a7

remove hearbeatclear function

b416d56

sfc-gh-ddas requested changes Feb 19, 2026

View reviewed changes

lib/client.c Show resolved Hide resolved

resolve the memory lick from snowflake print and remove duplicated file

a7bc328

sfc-gh-pbulawa requested changes Feb 24, 2026

View reviewed changes

fix typos

3d1974b

sfc-gh-ext-simba-jy requested review from sfc-gh-ddas and sfc-gh-pbulawa February 25, 2026 19:40

sfc-gh-pbulawa reviewed Mar 11, 2026

View reviewed changes

sfc-gh-ext-simba-jy and others added 2 commits March 12, 2026 10:52

Merge branch 'master' into SNOW-1737940

5bd13ec

fix the workflow

bce8ecb

sfc-gh-ext-simba-jy requested a review from sfc-gh-pbulawa March 13, 2026 01:34

sfc-gh-ext-simba-jy and others added 6 commits March 13, 2026 11:45

Merge branch 'master' into SNOW-1737940

2632721

fix race condition issues and minor error

85c6d89

Merge branch 'SNOW-1737940' of https://github.com/snowflakedb/libsnow…

bf2cf97

…flakeclient into SNOW-1737940

fix

4e25056

erase connections if the renewing failed

36aad40

remove unnecessary item from the queue

4134ac2

sfc-gh-pbulawa requested changes Mar 16, 2026

View reviewed changes

sfc-gh-pbulawa requested review from Copilot and removed request for sfc-gh-dstempniak March 16, 2026 15:48

Copilot started reviewing on behalf of sfc-gh-pbulawa March 16, 2026 15:49 View session

Copilot AI reviewed Mar 16, 2026

View reviewed changes

	sf_bool STDCALL create_header(SF_CONNECT sf, SF_HEADER header, SF_ERROR_STRUCT *error) {
	sf_bool ret = SF_BOOLEAN_FALSE;
	size_t header_token_size;
	size_t header_direct_query_token_size;
	size_t header_service_name_size;
	const char *token = header->renew_session ? sf->master_token : sf->token;

	assert_false(sf_strncasecmp(previous_masterToken, sf->master_token, strlen(sf->token)) == 0);
	assert_false(sf_strncasecmp(previous_masterToken, sf->master_token, strlen(sf->master_token)) == 0);

Conversation

sfc-gh-ext-simba-jy commented Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfc-gh-ext-simba-jy Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sfc-gh-pbulawa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sfc-gh-ext-simba-jy commented Mar 8, 2025 •

edited

Loading

sfc-gh-ext-simba-jy Mar 12, 2026 •

edited

Loading