Implemented thread handling for WaitForMultipleObjects. #1964

akallabeth · 2014-07-15T07:18:35Z

Implemented thread specific functions:

Thread debugging (open handles, ...) now available via CMake flag
** Add DumpThreadHandles wherever you want to know the state of thread handles
CloseHandle now properly supports threads via registered close function
WaitForSingleObject and WaitForMultipleObjects now support threads in INFINITE and timed wait.
WaitForMultipleObjects bWaitAll now supported
_GetCurrentThread now actually returns a valid handle.
Thread pool cleanup fixed, now properly cleaning up resources in ArrayList free function and exiting thread.
pthread_mutex_timedlock now using actual timeout instead of 10ms polling.

freerdp-bot · 2014-07-15T09:18:58Z

Test FAILed.
Refer to this link for build results: https://ci.freerdp.com/job/PullRequestBuilder/474/

freerdp-bot · 2014-07-15T09:26:36Z

Test FAILed.
Refer to this link for build results: https://ci.freerdp.com/job/PullRequestBuilder/473/

freerdp-bot · 2014-07-15T10:26:27Z

Test FAILed.
Refer to this link for build results: https://ci.freerdp.com/job/PullRequestBuilder/475/

freerdp-bot · 2014-07-15T10:45:05Z

Test FAILed.
Refer to this link for build results: https://ci.freerdp.com/job/PullRequestBuilder/476/

akallabeth · 2014-07-15T10:45:33Z

@freerdp-bot test

freerdp-bot · 2014-07-15T10:52:55Z

Test PASSed.
Refer to this link for build results: https://ci.freerdp.com/job/PullRequestBuilder/477/

freerdp-bot · 2014-07-15T12:47:59Z

Test FAILed.
Refer to this link for build results: https://ci.freerdp.com/job/PullRequestBuilder/478/

MartinHaimberger · 2014-07-15T12:52:57Z

+1, code looks good besides the failing test

freerdp-bot · 2014-07-15T12:58:57Z

Test PASSed.
Refer to this link for build results: https://ci.freerdp.com/job/PullRequestBuilder/479/

akallabeth · 2014-07-15T12:59:13Z

@MartinHaimberger Last commit fixes failing tests.

freerdp-bot · 2014-07-15T13:05:57Z

Test PASSed.
Refer to this link for build results: https://ci.freerdp.com/job/PullRequestBuilder/480/

bmiklautz · 2014-07-15T23:21:47Z

winpr/libwinpr/thread/thread.c

+
+	fprintf(stderr, "---------------- End Dumping thread handles -------------\n");
+}
+#endif
 #endif


I really do like this idea!

akallabeth · 2014-07-16T10:13:29Z

@bmiklautz @MartinHaimberger Added support for bWaitAll and fixed bugs found during testing, looks more solid now. Could you please confirm?

freerdp-bot · 2014-07-16T10:15:34Z

Test FAILed.
Refer to this link for build results: https://ci.freerdp.com/job/PullRequestBuilder/482/

freerdp-bot · 2014-07-16T10:16:05Z

Test FAILed.
Refer to this link for build results: https://ci.freerdp.com/job/PullRequestBuilder/483/

akallabeth · 2014-07-16T10:17:47Z

@freerdp-bot test

freerdp-bot · 2014-07-16T10:23:44Z

Test PASSed.
Refer to this link for build results: https://ci.freerdp.com/job/PullRequestBuilder/484/

akallabeth · 2014-07-17T09:41:07Z

@bmiklautz @MartinHaimberger @awakecoding @mfleisz @nfedera Did some more testing, working quite well now. Could you check if it works for you as well?

freerdp-bot · 2014-07-17T09:47:51Z

Test PASSed.
Refer to this link for build results: https://ci.freerdp.com/job/PullRequestBuilder/488/

hardening · 2014-07-17T13:13:35Z

winpr/libwinpr/thread/thread.c

+
+#endif
+}
+
 /**
 * TODO: implement thread suspend/resume using pthreads
 * http://stackoverflow.com/questions/3140867/suspend-pthreads-without-using-condition
 */


I've read the article and I really don't know who would want a pthread_suspend() function. When a thread gets suspended it should always be a volontary action (from the thread). Otherwise you can get suspended with some mutex hold and lead to a deadlock.

@hardening if I'm not mistaken windows can create a suspended thread so I guess that why one want that in winpr.

hardening · 2014-07-17T13:17:43Z

With the recent changes around monolithic, can't a Thread have an Event instead of dupllicating the Event code in Thread ?

hardening · 2014-07-17T13:20:09Z

except my remark LGTM, +1

akallabeth · 2014-07-28T06:31:37Z

@hardening could probably be done. Just didn't want the dependency on event in thread to avoid the effort (recursive calls, ...) for testing.

freerdp-bot · 2014-07-28T06:38:57Z

Test PASSed.
Refer to this link for build results: https://ci.freerdp.com/job/PullRequestBuilder/513/

freerdp-bot · 2014-08-20T12:57:34Z

Test FAILed.
Refer to this link for build results: https://ci.freerdp.com/job/PullRequestBuilder/566/

freerdp-bot · 2014-08-20T12:57:36Z

Test FAILed.
Refer to this link for build results: https://ci.freerdp.com/job/PullRequestBuilder/565/

akallabeth · 2014-08-20T13:51:27Z

@freerdp-bot test

…llowed.

Fixed setting of started flag for threads. pthred_exit now used correct return value argument. Return value now also set when terminating thread.

…ects.

Implemented bWaitAll for WaitForMultipleObjects. pthread_join now only called on first wait event, later ones skip this to avoid undefined behavior.

freerdp-bot · 2014-10-27T10:55:40Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestBuilder/651/Test FAILed.

bmiklautz · 2014-10-27T13:55:49Z

@freerdp-bot test

freerdp-bot · 2014-10-27T14:02:22Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestBuilder/652/Test FAILed.

bmiklautz · 2014-10-27T14:54:54Z

@freerdp-bot test

freerdp-bot · 2014-10-27T15:01:02Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestBuilder/653/Test PASSed.

hardening · 2014-10-31T09:39:18Z

winpr/libwinpr/synch/wait.c


-			thread->started = FALSE;
+		if (!thread->joined)


There's a race condition on this variable if 2 threads call WaitForSingleObject() concurrently.
pthread_join() is atomically correct with this, so we should call it inconditionaly, and we can remove this struct member too (joined).

hardening · 2014-10-31T09:39:59Z

@akallabeth sorry to comment so late.

mfleisz · 2014-10-31T10:14:20Z

@akallabeth: Looks good despite the two comments from @hardening. If this is fixed I'll merge since this has been open for ages and nobody seems to have any objections.

…onditions.

akallabeth · 2014-11-03T08:33:53Z

So, fixed the races. The flag is still there (preventing multiple waits to call pthread_join, which is undefined) but it is locked by a mutex now.
@hardening better late than never, no problem ;)
@mfleisz Ok, looking forward to that.

freerdp-bot · 2014-11-03T08:37:26Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestBuilder/662/Test PASSed.

hardening · 2014-11-03T10:08:55Z

pthread_join() is a real pain, it's functional only for trivial, limited, cases, which is of course the one you never hit in real programs. I don't know why they gave such a stupid definition for pthread_join()...

Anyway I guess that in
pthread_lock()
.... pthread_join()
pthread_unlock()

We could use a pthread_condition stored in the thread to accomplish the same operation and allowing multiple thread to wait for completion. The pthread_condition would also allow to do a wait with timeout. It's not a brilliant idea from me it's just that many toolkit are doing it this way (at least Qt, and others)

hardening · 2014-11-10T08:32:56Z

winpr/libwinpr/synch/wait.c

+						{
+							WLog_ERR(TAG, "pthread_join failure: [%d] %s",
+									 status, strerror(status));
+							return WAIT_FAILED;


In this exit path we're still holding the thread->mutex lock

freerdp-bot · 2014-11-10T09:32:20Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestBuilder/671/Test PASSed.

Implemented thread handling for WaitForMultipleObjects.

akallabeth mentioned this pull request Jul 15, 2014

Replace select loop in clients with WaitForMultipleObjects #1910

Closed

bmiklautz reviewed Jul 15, 2014
View reviewed changes

hardening reviewed Jul 17, 2014
View reviewed changes

akallabeth mentioned this pull request Aug 6, 2014

Remove remaining select in client code and fix *get_fds functions #2021

Closed

akallabeth force-pushed the thread-wait-support branch from 3a30f2e to e73e64d Compare August 20, 2014 12:36

akallabeth added 10 commits October 27, 2014 11:23

Removed WaitForSingleObject in reset_event, joining thread when not a…

56ae609

…llowed.

Added funcion name to debug messages.

5122ce9

Fixed setting of started flag for threads. pthred_exit now used correct return value argument. Return value now also set when terminating thread.

Added additional tests for WaitForSingleObject and WaitForMultipleObj…

c5bb612

…ects.

Error messages now prefixed with function name.

7acdbda

Implemented bWaitAll for WaitForMultipleObjects. pthread_join now only called on first wait event, later ones skip this to avoid undefined behavior.

Added joined flag.

7ef2bd3

Setting exited flag in TerminateThread now to allow setting return code.

02a4d77

Fixed typo

c6ed95d

Fixed ExitThread, now running pthread_exit and setting event.

a6eff65

Fixed merge errors.

8f98f28

Fixed missing includes and typo.

5bf25fd

akallabeth force-pushed the thread-wait-support branch from 3afa77b to 5bf25fd Compare October 27, 2014 10:48

hardening reviewed Oct 31, 2014
View reviewed changes

Locking thread until joined now, preventing multiple joins and race c…

017e4a3

…onditions.

hardening reviewed Nov 10, 2014
View reviewed changes

Fixed unlocking if pthread_join fails.

ff3fcc2

mfleisz added a commit that referenced this pull request Nov 11, 2014

Merge pull request #1964 from akallabeth/thread-wait-support

6cd55a5

Implemented thread handling for WaitForMultipleObjects.

mfleisz merged commit 6cd55a5 into FreeRDP:master Nov 11, 2014

akallabeth deleted the thread-wait-support branch November 11, 2014 12:09

Implemented thread handling for WaitForMultipleObjects. #1964

Implemented thread handling for WaitForMultipleObjects. #1964

Conversation

akallabeth commented Jul 15, 2014

freerdp-bot commented Jul 15, 2014

freerdp-bot commented Jul 15, 2014

freerdp-bot commented Jul 15, 2014

freerdp-bot commented Jul 15, 2014

akallabeth commented Jul 15, 2014

freerdp-bot commented Jul 15, 2014

freerdp-bot commented Jul 15, 2014

MartinHaimberger commented Jul 15, 2014

freerdp-bot commented Jul 15, 2014

akallabeth commented Jul 15, 2014

freerdp-bot commented Jul 15, 2014

bmiklautz Jul 15, 2014

Choose a reason for hiding this comment

akallabeth commented Jul 16, 2014

freerdp-bot commented Jul 16, 2014

freerdp-bot commented Jul 16, 2014

akallabeth commented Jul 16, 2014

freerdp-bot commented Jul 16, 2014

akallabeth commented Jul 17, 2014

freerdp-bot commented Jul 17, 2014

hardening Jul 17, 2014

Choose a reason for hiding this comment

bmiklautz Sep 15, 2014

Choose a reason for hiding this comment

hardening commented Jul 17, 2014

hardening commented Jul 17, 2014

akallabeth commented Jul 28, 2014

freerdp-bot commented Jul 28, 2014

freerdp-bot commented Aug 20, 2014

freerdp-bot commented Aug 20, 2014

akallabeth commented Aug 20, 2014

freerdp-bot commented Oct 27, 2014

bmiklautz commented Oct 27, 2014

freerdp-bot commented Oct 27, 2014

bmiklautz commented Oct 27, 2014

freerdp-bot commented Oct 27, 2014

hardening Oct 31, 2014

Choose a reason for hiding this comment

hardening commented Oct 31, 2014

mfleisz commented Oct 31, 2014

akallabeth commented Nov 3, 2014

freerdp-bot commented Nov 3, 2014

hardening commented Nov 3, 2014

hardening Nov 10, 2014

Choose a reason for hiding this comment

freerdp-bot commented Nov 10, 2014