add fix timer race code #412

M-iceberg · 2024-11-14T03:15:38Z

No description provided.

M-iceberg · 2024-11-14T03:15:49Z

/azp run #Resolved

azure-pipelines · 2024-11-14T03:16:03Z

Azure Pipelines successfully started running 1 pipeline(s). #Resolved

M-iceberg · 2024-11-19T00:07:57Z

/azp run #Resolved

azure-pipelines · 2024-11-19T00:08:11Z

Azure Pipelines successfully started running 1 pipeline(s). #Resolved

darobs · 2024-11-19T00:51:59Z

interfaces/inc/c_pal/threadpool.h

@@ -21,10 +21,13 @@ extern "C" {
 #endif

 typedef struct THREADPOOL_TAG THREADPOOL;
-typedef struct TIMER_INSTANCE_TAG* TIMER_INSTANCE_HANDLE;
+typedef struct TIMER_INSTANCE_TAG TIMER_INSTANCE;
+typedef struct TIMER_INSTANCE_TAG* TIMER_INSTANCE_HANDLE; //todo: if this one is needed


should probably remove TIMER_INSTANCE_HANDLE #Resolved

Not removed, I can still see it.

darobs · 2024-11-19T00:54:05Z

linux/devdoc/threadpool_linux_requirements.md


-MOCKABLE_FUNCTION(, int, threadpool_timer_restart, TIMER_INSTANCE_HANDLE, timer, uint32_t, start_delay_ms, uint32_t, timer_period_ms);
+MOCKABLE_FUNCTION(, int, threadpool_timer_restart, THANDLE(TIMER_INSTANCE)*, timer, uint32_t, start_delay_ms, uint32_t, timer_period_ms);


)*

timer is just THANDLE(TIMER_INSTANCE), not THANDLE(TIMER_INSTANCE)* Same for line 92. #Resolved

darobs · 2024-11-19T01:28:47Z

linux/tests/threadpool_linux_ut/threadpool_linux_ut.c

@@ -1140,9 +1141,10 @@ TEST_FUNCTION(threadpool_timer_start_with_NULL_work_function_context_succeeds)
    // arrange
    THANDLE(THREADPOOL) threadpool = test_create_and_open_threadpool();

-    TIMER_INSTANCE_HANDLE timer_instance;
+    THANDLE(TIMER_INSTANCE) timer_instance = NULL;


timer_instance = NULL

Do we have to assign this to be NULL for threadpool_timer_start to work? I can see this might be an annoyance when the timer is part of a struct, like it generally is. #Resolved

will getting segmentation fault if not set this THANDLE to NULL

The common pattern we have in the code that uses this is

typedef struct SOME_STRUCT_TAG { // ... THANDLE(TIMER_INSTANCE) timer; // ... } SOME_STRUCT; // then in the "_create" function: SOME_STRUCT_HANDLE result = (SOME_STRUCT_HANDLE)malloc(sizeof(SOME_STRUCT); threadpool_timer_start(threadpool, start_delay, period, test_work_function, context, &result->timer);

The timer is being passed in uninitialized, and so requiring it to be initialized in the test implies that the above pattern will need to change. BTW, this comment goes along with using THANDLE_INITIALIZE_MOVE instead of THANDLE_MOVE

modified to initialize_move

darobs · 2024-11-19T01:35:59Z

win32/src/threadpool_win32.c

+                        threadpool_internal_set_timer(tp_timer, start_delay_ms, timer_period_ms);
+
+                        /* Codes_SRS_THREADPOOL_WIN32_42_009: [ threadpool_timer_start shall return the allocated handle in timer_handle. ]*/
+                        THANDLE_MOVE(TIMER_INSTANCE)(timer_handle, &timer_temp);


THANDLE_MOVE(TIMER_INSTANCE)(timer_handle, &timer_temp);

Would THANDLE_INITIALIZE_MOVE make more sense here? #Resolved

Same for Linux?

darobs · 2024-11-19T02:22:47Z

linux/linux_reals/real_threadpool.h

@@ -18,8 +18,7 @@
        threadpool_schedule_work, \
        threadpool_timer_start, \
        threadpool_timer_restart, \
-        threadpool_timer_cancel, \
-        threadpool_timer_destroy \
+        threadpool_timer_cancel
    ) \


build failed, missing \ at the end of this line #Resolved

azure-pipelines · 2024-11-19T21:12:23Z

No pipelines are associated with this pull request. #Resolved

azure-pipelines · 2024-11-19T21:12:42Z

No pipelines are associated with this pull request. #Resolved

M-iceberg · 2024-11-19T22:25:12Z

/azp run #Resolved

azure-pipelines · 2024-11-19T22:25:25Z

Azure Pipelines successfully started running 1 pipeline(s). #Resolved

M-iceberg · 2024-11-19T23:37:53Z

/azp run #Resolved

azure-pipelines · 2024-11-19T23:38:03Z

Azure Pipelines successfully started running 1 pipeline(s). #Resolved

dcristoloveanu · 2024-11-20T00:34:37Z

interfaces/inc/c_pal/threadpool.h

@@ -21,10 +21,13 @@ extern "C" {
 #endif

 typedef struct THREADPOOL_TAG THREADPOOL;
+typedef struct TIMER_INSTANCE_TAG TIMER_INSTANCE;
 typedef struct TIMER_INSTANCE_TAG* TIMER_INSTANCE_HANDLE;


TIMER_INSTANCE_HANDLE

Who uses the TIMER_INSTANCE_HANDLE? I am imagining that after this change there should be only one, right? #Resolved

Or are there more PRs planned? If so, we should add a comment explaining that, otherwise it is sort of confusing.

removed the timer_instance_handle, not using anymore

azure-pipelines · 2024-12-19T21:36:24Z

Azure Pipelines successfully started running 1 pipeline(s). #Resolved

M-iceberg · 2024-12-19T22:18:04Z

/azp run #Resolved

azure-pipelines · 2024-12-19T22:18:18Z

Azure Pipelines successfully started running 1 pipeline(s). #Resolved

dcristoloveanu · 2024-12-20T05:42:54Z

interfaces/devdoc/threadpool_requirements.md

@@ -39,13 +39,13 @@ MOCKABLE_FUNCTION(, int, threadpool_schedule_work_item, THANDLE(THREADPOOL), thr

 MOCKABLE_FUNCTION(, int, threadpool_schedule_work, THANDLE(THREADPOOL), threadpool, THREADPOOL_WORK_FUNCTION, work_function, void*, work_function_context);

-MOCKABLE_FUNCTION(, int, threadpool_timer_start, THANDLE(THREADPOOL), threadpool, uint32_t, start_delay_ms, uint32_t, timer_period_ms, THREADPOOL_WORK_FUNCTION, work_function, void*, work_function_context, TIMER_INSTANCE_HANDLE*, timer_handle);
+MOCKABLE_FUNCTION(, int, threadpool_timer_start, THANDLE(THREADPOOL), threadpool, uint32_t, start_delay_ms, uint32_t, timer_period_ms, THREADPOOL_WORK_FUNCTION, work_function, void*, work_function_context, TIMER **, timer_handle);


TIMER **

This is not what is in the header.
This should be a copy/paste of what we have in the threadpool.h #Resolved

done updated this md

dcristoloveanu · 2024-12-20T05:43:55Z

interfaces/inc/c_pal/threadpool.h

 typedef struct THREADPOOL_WORK_ITEM_TAG THREADPOOL_WORK_ITEM;
 typedef void (*THREADPOOL_WORK_FUNCTION)(void* context);

+THANDLE_TYPE_DECLARE(TIMER);


TIMER

I think this should be named THREADPOOL_TIMER. To be in sync with THREADPOOL_WORK_FUNCTION #Resolved

dcristoloveanu · 2024-12-20T05:44:22Z

interfaces/inc/c_pal/threadpool.h

@@ -21,10 +21,12 @@ extern "C" {
 #endif

 typedef struct THREADPOOL_TAG THREADPOOL;
-typedef struct TIMER_INSTANCE_TAG* TIMER_INSTANCE_HANDLE;
+typedef struct TIMER_TAG TIMER;


Can we have this close to the THANDLE_TYPE_DECLARE, so that it is not intermingled with the work item stuff? #Resolved

dcristoloveanu · 2024-12-20T05:44:45Z

linux/devdoc/threadpool_linux_requirements.md

@@ -65,7 +65,7 @@ The guard prevents any work from happening, but the event callback can still be

 ```C
 typedef struct THREADPOOL_TAG THREADPOOL;
-typedef struct TIMER_INSTANCE_TAG* TIMER_INSTANCE_HANDLE;
+typedef struct TIMER_TAG* TIMER *;


This is not what is in the header ... #Resolved

dcristoloveanu · 2024-12-20T05:45:09Z

linux/linux_reals/real_threadpool.h

-    void real_threadpool_timer_cancel(TIMER_INSTANCE_HANDLE timer);
-    void real_threadpool_timer_destroy(TIMER_INSTANCE_HANDLE timer);
+    int real_threadpool_timer_start(THANDLE(THREADPOOL) threadpool, uint32_t start_delay_ms, uint32_t timer_period_ms, THREADPOOL_WORK_FUNCTION work_function, void* work_function_context, THANDLE(TIMER)* timer_handle);
+    int real_threadpool_timer_restart(THANDLE(TIMER) timer, uint32_t start_delay_ms, uint32_t timer_period_ms);


uint32_t

Suggest including stdint.h #Resolved

dcristoloveanu · 2024-12-20T05:45:32Z

linux/linux_reals/real_threadpool_renames.h

@@ -14,7 +14,6 @@
 #define threadpool_timer_start          real_threadpool_timer_start
 #define threadpool_timer_restart        real_threadpool_timer_restart
 #define threadpool_timer_cancel         real_threadpool_timer_cancel
-#define threadpool_timer_destroy        real_threadpool_timer_destroy


Neat-o #Resolved

dcristoloveanu · 2024-12-20T05:47:30Z

linux/src/threadpool_linux.c

 } THREADPOOL_TASK;

 typedef struct THREADPOOL_WORK_ITEM_TAG
 {
    THREADPOOL_WORK_FUNCTION work_function;
    void* work_function_ctx;
-} THREADPOOL_WORK_ITEM, *THREADPOOL_WORK_ITEM_HANDLE;
+    volatile_atomic int32_t pending_work_item_count;
+} THREADPOOL_WORK_ITEM, * THREADPOOL_WORK_ITEM_HANDLE;


This was removed by Nishikant, so it was likely added back by mistake during merge. #Resolved

dcristoloveanu · 2024-12-20T05:47:53Z

linux/src/threadpool_linux.c


 typedef struct THREADPOOL_TASK_TAG
 {
    volatile_atomic int32_t task_state;
    THREADPOOL_WORK_FUNCTION work_function;
    void* work_function_ctx;
+    volatile uint32_t pending_api_calls;
+    volatile_atomic int32_t *pending_work_item_count_ptr;
 } THREADPOOL_TASK;


I think this was removed by Nishikant also. #Resolved

dcristoloveanu · 2024-12-20T05:49:29Z

linux/src/threadpool_linux.c

+    // and manage each state as in-use or not. That way the timer data allocation is not dependent on threadpool_timer_start/destroy.
+    // To reproduce this problem, create a large number of threads that delete the timer at the same time it is due to expire and remove the pause
+    // above.
+}


This mentions a small wait that does not exist any longer. I think this comment is now obsolete? #Resolved

dcristoloveanu · 2024-12-20T05:50:18Z

linux/src/threadpool_linux.c

            {
-                LogErrorNo("Failure calling timer_create.");
+                LogError("srw_lock_ll_init failed");
            }


I would expect a Codes for "if any error occurrs then threadpool_timer_start shall fail and return a non zero value. #Resolved

dcristoloveanu · 2024-12-20T05:51:00Z

linux/src/threadpool_linux.c

 {
    int result;
    if (timer == NULL)
    {
        /* Codes_SRS_THREADPOOL_LINUX_07_064: [ If timer is NULL, threadpool_timer_restart shall fail and return a non-zero value. ]*/
-        LogError("Invalid args: TIMER_INSTANCE_HANDLE timer = %p, uint32_t start_delay_ms = %" PRIu32 ", uint32_t timer_period_ms = %" PRIu32 "",
+        LogError("Invalid args: TIMER * timer = %p, uint32_t start_delay_ms = %" PRIu32 ", uint32_t timer_period_ms = %" PRIu32 "",


TIMER *

Signature above is with THANLDE(TIMER) #Resolved

dcristoloveanu · 2024-12-20T05:51:23Z

linux/src/threadpool_linux.c

 {
    if (timer == NULL)
    {
        /* Codes_SRS_THREADPOOL_LINUX_07_068: [ If timer is NULL, threadpool_timer_cancel shall fail and return. ]*/
-        LogError("Invalid args: TIMER_INSTANCE_HANDLE timer = %p", timer);
+        LogError("Invalid args: TIMER * timer = %p", timer);


TIMER *

This too: THANDLE(TOIMER) #Resolved

dcristoloveanu · 2024-12-20T05:52:14Z

#include <string.h>

Didn't we say we should have only one int test file?

Refers to: linux/tests/threadpool_linux_int/threadpool_linux_int.c:8 in 438a45a. [](commit_id = 438a45a, deletion_comment = False)

dcristoloveanu · 2024-12-20T05:52:46Z

win32/devdoc/threadpool_win32.md

@@ -12,7 +12,7 @@ It uses the PTP_POOL associated with the execution engine passed as argument to

 ```c
 typedef struct THREADPOOL_TAG THREADPOOL;
-typedef struct TIMER_INSTANCE_TAG* TIMER_INSTANCE_HANDLE;
+typedef struct TIMER_TAG* TIMER *;


ER_TAG* TIMER *;

Looks off comparing to the header #Resolved

azure-pipelines · 2024-12-23T19:12:54Z

No pipelines are associated with this pull request. #Resolved

azure-pipelines · 2024-12-23T19:14:53Z

No pipelines are associated with this pull request. #Resolved

azure-pipelines · 2024-12-23T19:15:43Z

No pipelines are associated with this pull request. #Resolved

azure-pipelines · 2024-12-23T19:15:54Z

No pipelines are associated with this pull request. #Resolved

azure-pipelines · 2024-12-23T19:16:00Z

No pipelines are associated with this pull request. #Resolved

M-iceberg · 2024-12-24T01:08:41Z

/azp run

azure-pipelines · 2024-12-24T01:08:55Z

Azure Pipelines successfully started running 1 pipeline(s).

M-iceberg · 2024-12-27T01:11:13Z

/azp run

azure-pipelines · 2024-12-27T01:11:26Z

Azure Pipelines successfully started running 1 pipeline(s).

add fix timer race code

81f5069

microsoft-github-policy-service bot requested review from anporumb, avranju, mattdurak, dcristoloveanu, jebrando, darobs, ipg2013 and parth21999 November 14, 2024 03:16

upload linux code change

9284da2

darobs reviewed Nov 19, 2024

View reviewed changes

M-iceberg added 2 commits November 19, 2024 14:22

address comments

70d23f1

small fix

1d74206

microsoft-github-policy-service bot requested a review from darobs November 19, 2024 22:25

fix linux build

491e0ff

dcristoloveanu reviewed Nov 20, 2024

View reviewed changes

fix memory leak due to merge

438a45a

dcristoloveanu reviewed Dec 20, 2024

View reviewed changes

address comments and change start timer signature

9823258

microsoft-github-policy-service bot requested a review from dcristoloveanu December 24, 2024 01:08

fix memory leak

b619403


		MOCKABLE_FUNCTION(, int, threadpool_timer_restart, TIMER_INSTANCE_HANDLE, timer, uint32_t, start_delay_ms, uint32_t, timer_period_ms);
		MOCKABLE_FUNCTION(, int, threadpool_timer_restart, THANDLE(TIMER_INSTANCE)*, timer, uint32_t, start_delay_ms, uint32_t, timer_period_ms);

add fix timer race code #412

Are you sure you want to change the base?

add fix timer race code #412

Conversation

M-iceberg commented Nov 14, 2024

M-iceberg commented Nov 14, 2024 • edited Loading

azure-pipelines bot commented Nov 14, 2024 • edited by M-iceberg Loading

M-iceberg commented Nov 19, 2024 • edited Loading

azure-pipelines bot commented Nov 19, 2024 • edited by M-iceberg Loading

darobs Nov 19, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darobs Nov 19, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darobs Nov 19, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darobs Nov 19, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darobs Nov 19, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azure-pipelines bot commented Nov 19, 2024 • edited by M-iceberg Loading

azure-pipelines bot commented Nov 19, 2024 • edited by M-iceberg Loading

M-iceberg commented Nov 19, 2024 • edited Loading

azure-pipelines bot commented Nov 19, 2024 • edited by M-iceberg Loading

M-iceberg commented Nov 19, 2024 • edited Loading

azure-pipelines bot commented Nov 19, 2024 • edited by M-iceberg Loading

dcristoloveanu Nov 20, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azure-pipelines bot commented Dec 19, 2024 • edited by M-iceberg Loading

M-iceberg commented Dec 19, 2024 • edited Loading

azure-pipelines bot commented Dec 19, 2024 • edited by M-iceberg Loading

dcristoloveanu Dec 20, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcristoloveanu Dec 20, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcristoloveanu Dec 20, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcristoloveanu Dec 20, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcristoloveanu Dec 20, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcristoloveanu Dec 20, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

dcristoloveanu Dec 20, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcristoloveanu Dec 20, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcristoloveanu Dec 20, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcristoloveanu Dec 20, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcristoloveanu Dec 20, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcristoloveanu Dec 20, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcristoloveanu commented Dec 20, 2024

dcristoloveanu Dec 20, 2024 • edited by M-iceberg Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M-iceberg commented Nov 14, 2024 •

edited

Loading

azure-pipelines bot commented Nov 14, 2024 •

edited by M-iceberg

Loading

M-iceberg commented Nov 19, 2024 •

edited

Loading

azure-pipelines bot commented Nov 19, 2024 •

edited by M-iceberg

Loading

darobs Nov 19, 2024 •

edited by M-iceberg

Loading

darobs Nov 19, 2024 •

edited by M-iceberg

Loading

darobs Nov 19, 2024 •

edited by M-iceberg

Loading

darobs Nov 19, 2024 •

edited by M-iceberg

Loading

darobs Nov 19, 2024 •

edited by M-iceberg

Loading

azure-pipelines bot commented Nov 19, 2024 •

edited by M-iceberg

Loading

azure-pipelines bot commented Nov 19, 2024 •

edited by M-iceberg

Loading

M-iceberg commented Nov 19, 2024 •

edited

Loading

azure-pipelines bot commented Nov 19, 2024 •

edited by M-iceberg

Loading

M-iceberg commented Nov 19, 2024 •

edited

Loading

azure-pipelines bot commented Nov 19, 2024 •

edited by M-iceberg

Loading

dcristoloveanu Nov 20, 2024 •

edited by M-iceberg

Loading

azure-pipelines bot commented Dec 19, 2024 •

edited by M-iceberg

Loading

M-iceberg commented Dec 19, 2024 •

edited

Loading

azure-pipelines bot commented Dec 19, 2024 •

edited by M-iceberg

Loading

dcristoloveanu Dec 20, 2024 •

edited by M-iceberg

Loading

dcristoloveanu Dec 20, 2024 •

edited by M-iceberg

Loading

dcristoloveanu Dec 20, 2024 •

edited by M-iceberg

Loading

dcristoloveanu Dec 20, 2024 •

edited by M-iceberg

Loading

dcristoloveanu Dec 20, 2024 •

edited by M-iceberg

Loading

dcristoloveanu Dec 20, 2024 •

edited by M-iceberg

Loading

dcristoloveanu Dec 20, 2024 •

edited by M-iceberg

Loading

dcristoloveanu Dec 20, 2024 •

edited by M-iceberg

Loading

dcristoloveanu Dec 20, 2024 •

edited by M-iceberg

Loading

dcristoloveanu Dec 20, 2024 •

edited by M-iceberg

Loading

dcristoloveanu Dec 20, 2024 •

edited by M-iceberg

Loading

dcristoloveanu Dec 20, 2024 •

edited by M-iceberg

Loading

dcristoloveanu Dec 20, 2024 •

edited by M-iceberg

Loading

azure-pipelines bot commented Dec 23, 2024 •

edited by M-iceberg

Loading

azure-pipelines bot commented Dec 23, 2024 •

edited by M-iceberg

Loading

azure-pipelines bot commented Dec 23, 2024 •

edited by M-iceberg

Loading

azure-pipelines bot commented Dec 23, 2024 •

edited by M-iceberg

Loading

azure-pipelines bot commented Dec 23, 2024 •

edited by M-iceberg

Loading