Replace pthread with CThreads #141

ThePedroo · 2023-02-10T23:50:12Z

Notice

I understand the code that I have edited, and have the means
to test it before making changes to Concord.

What?

This PR replaces the pthread by cthread (a library that uses both pthread and Windows Threads).

Why?

To improve compability with native Windows, and to not be necessary to use pthread on Windows.

How?

All parts where pthread is used, was replaced with cthreads functions.

Testing?

Basic testing using Concord's testing was used, on Windows and Linux (Arch Linux), but more testings must be done.

lcsmuller

This looks very promising, good progress so far. I've left a couple questions and suggestions regarding the implementation, but mostly nitpicks!

core/cthreads.c

lcsmuller · 2023-02-11T13:49:53Z

src/discord-voice.c

@@ -643,6 +643,8 @@ discord_voice_join(struct discord *client,
                   bool self_mute,
                   bool self_deaf)
 {
+    client_lock = __cthreads_mutex_init();


I'm unsure about this one.. does Windows have a PTHREAD_MUTEX_INITIALIZER equivalent? You can try searching for "initializing global mutex Windows"

From what I have seen no, according to this stack overflow answer
I wonder if you really need it, because if yes we can check if it exists in every cthreads action, so it will work anyways, but for using voice they will first use that one anyways, are they?

core/cthreads.c

core/cthreads.h

HackerSmacker

I know it's a ton of work to replace those ifdefs, but, I feel it may be a better idea to use osname in lieu of #else, since that feels like a "solution of last resort."

core/cthreads.h

core/threadpool.c

core/cthreads.c

ThePedroo · 2023-02-11T16:04:51Z

Woah! That was... insane, well, I've applied the suggestions I've understood and fixed some errors and improved the functions, it should be better now.. although I need to test it on Windows.. I'm going to do later since now I'm a little tired for testing, but should work on Linux

ThePedroo · 2023-02-11T18:22:25Z

I've got some bugs fixed (mainly on Windows threads but also on pthreads), now pthread is working fine, although I'm still dealing with cygwin "exception" with pthread & windows threads, I'm going to try to use MSYS2 and see if it works (It didn't work, it still uses cygwin for some wild reason)

ThePedroo · 2023-05-15T00:33:17Z

I'm going to check the ones I didn't really solve in the reviews soonly, but is anybody available to test it? I got it working on cygwin (manually removing the ifdefs to get the windows.h since for some reasons it doesn't set win flag) and should work on the rest

HackerSmacker · 2023-06-12T23:35:38Z

Okay, time for my Official Review of Cthreads:

TESTED COMPILERS:

MSVC 2022, 2013 (Windows 10, 8.1)
OpenWatcom from June 2022 (Windows NT 4)
Cygwin GCC 11.3.0 (Windows 10)
MinGW GCC 11.3.0 (Windows 10)
GCC 11.3.0 (FreeBSD 586)
Clang 10.0 (FreeBSD 586)
GCC 8.3.0 (Linux 4.19.0 System/390 custom image)
acomp SVR5 (UnixWare 7.1.1)

Works on all of those. There is only one issue, and, that's the dilemma of a warning that you get on line 72 of cthreads.c. If you use the plain DWORD cast, Watcom is happy, but, GNU compilers yell.

Also, I should mention that this is a test of compiling and linking Cthreads, NOT all of Concord!

ThePedroo · 2023-07-01T15:22:32Z

I've fixed some issues (in both pthread and Windows threads), but I'm still going to take a look in the Windows equivalents to be sure everything's alright

Tests are appreciated, but most things are the same, should still work

CThreads can also work in Windows using Windows threads, due to this, pthread was replaced by it. Co-authored-by: Lucas Müller <[email protected]> Co-authored-by: HackerSmacker <[email protected]>

HackerSmacker

Took me a minute, but, I think it should be good. All the tests I've done were good, so, I think it's good to go.

core/cthreads.c

test/cthreads.c

lcsmuller

LGTM, but we should merge this once the TLS issue is sorted out

This commit updates CThreads library with latest fixes.

Anotra · 2024-02-02T21:45:14Z

I've looked though the code, it's not fully featured, and contains some serious flaws. If you want to support windows threads, I'd look into something such as this https://github.com/GerHobbelt/pthread-win32 first.

ThePedroo · 2024-02-02T23:20:53Z

Thanks for the review! Could you provide me the list of the flaws?

I took a small look on pthread-win32, but I don't think it follows CThreads goals: uncomplicated "threading library", allowing cross-compatibility while having a small and easy (lib) codebase

About not being fully featured, CThreads was born to be for Concord primarily, and it covers all used pthread functions. I do plan to cover more parts

Thanks for the patience to review anyway

This commit improves CThreads usage by destroying properly the mutexes, and expanding its usage to examples.

This commit updates the library, allowing it to work properly on Windows.

ThePedroo · 2024-03-21T21:32:20Z

I was able to get back to my city where I have a Windows machine and fix Windows part issues. Now, with proper edits to make Cygwin to use Windows part, it works flawlessly

lcsmuller · 2024-12-07T18:49:10Z

To give you a little update on this, we will need to hold until we have an actual structure for unit testing, integration tests, e2e tests. Otherwise, its a bit too risky to merge on the repo's current state

ThePedroo force-pushed the feat/thread branch 2 times, most recently from 9ec25be to 7796fdb Compare February 11, 2023 13:25

lcsmuller reviewed Feb 11, 2023

View reviewed changes

core/cthreads.h Outdated Show resolved Hide resolved

HackerSmacker reviewed Feb 11, 2023

View reviewed changes

core/cthreads.h Show resolved Hide resolved

core/cthreads.h Outdated Show resolved Hide resolved

core/threadpool.c Show resolved Hide resolved

core/cthreads.c Outdated Show resolved Hide resolved

ThePedroo force-pushed the feat/thread branch 2 times, most recently from e024684 to bb1386f Compare February 11, 2023 16:03

ThePedroo force-pushed the feat/thread branch from bb1386f to 7dad488 Compare February 11, 2023 18:11

lcsmuller changed the title ~~feat(thread): Replace pthread to CThreads~~ feat(thread): Replace pthread with CThreads May 14, 2023

lcsmuller changed the title ~~feat(thread): Replace pthread with CThreads~~ [WIP] Replace pthread with CThreads May 14, 2023

ThePedroo force-pushed the feat/thread branch 3 times, most recently from 8955e85 to ea13c56 Compare May 16, 2023 17:27

ThePedroo force-pushed the feat/thread branch 6 times, most recently from c2345fc to c717805 Compare June 12, 2023 23:27

ThePedroo force-pushed the feat/thread branch 2 times, most recently from b318f52 to 51b3073 Compare June 12, 2023 23:47

ThePedroo force-pushed the feat/thread branch from 51b3073 to 4ddf6ef Compare July 1, 2023 15:19

feat(thread): Replace pthread to CThreads

3ffcdf0

CThreads can also work in Windows using Windows threads, due to this, pthread was replaced by it. Co-authored-by: Lucas Müller <[email protected]> Co-authored-by: HackerSmacker <[email protected]>

ThePedroo force-pushed the feat/thread branch from 4ddf6ef to 3ffcdf0 Compare August 31, 2023 02:31

Merge branch 'master' into feat/thread

1a8a517

lcsmuller changed the title ~~[WIP] Replace pthread with CThreads~~ Replace pthread with CThreads Sep 14, 2023

HackerSmacker approved these changes Sep 14, 2023

View reviewed changes

core/cthreads.c Outdated Show resolved Hide resolved

core/cthreads.c Outdated Show resolved Hide resolved

core/cthreads.c Outdated Show resolved Hide resolved

test/cthreads.c Show resolved Hide resolved

lcsmuller approved these changes Oct 1, 2023

View reviewed changes

ThePedroo force-pushed the feat/thread branch 6 times, most recently from ccb44d1 to 50c8f29 Compare February 1, 2024 05:09

update(cthreads): update CThreads library

61e9dc0

This commit updates CThreads library with latest fixes.

ThePedroo force-pushed the feat/thread branch from 50c8f29 to 61e9dc0 Compare February 1, 2024 07:37

ThePedroo changed the base branch from master to dev February 2, 2024 01:09

Merge branch 'dev' into feat/thread

7717c3d

ThePedroo force-pushed the feat/thread branch 3 times, most recently from f88f90d to 033bdc9 Compare February 2, 2024 16:17

chore(cthreads): improve usage

cafb3ff

This commit improves CThreads usage by destroying properly the mutexes, and expanding its usage to examples.

ThePedroo force-pushed the feat/thread branch from 033bdc9 to cafb3ff Compare February 3, 2024 01:23

ThePedroo force-pushed the feat/thread branch 5 times, most recently from 8c9569f to ef13fba Compare March 17, 2024 03:56

chore(cthreads): update library

b72dd51

This commit updates the library, allowing it to work properly on Windows.

ThePedroo force-pushed the feat/thread branch from ef13fba to b72dd51 Compare March 17, 2024 03:57

Merge branch 'dev' into feat/thread

5f30d4a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace pthread with CThreads #141

Replace pthread with CThreads #141

ThePedroo commented Feb 10, 2023

lcsmuller left a comment

lcsmuller Feb 11, 2023

ThePedroo Jun 12, 2023

HackerSmacker left a comment

ThePedroo commented Feb 11, 2023

ThePedroo commented Feb 11, 2023 •

edited

Loading

ThePedroo commented May 15, 2023

HackerSmacker commented Jun 12, 2023

ThePedroo commented Jul 1, 2023

HackerSmacker left a comment

lcsmuller left a comment

Anotra commented Feb 2, 2024

ThePedroo commented Feb 2, 2024

ThePedroo commented Mar 21, 2024

lcsmuller commented Dec 7, 2024 •

edited

Loading

Replace pthread with CThreads #141

Are you sure you want to change the base?

Replace pthread with CThreads #141

Conversation

ThePedroo commented Feb 10, 2023

Notice

What?

Why?

How?

Testing?

lcsmuller left a comment

Choose a reason for hiding this comment

lcsmuller Feb 11, 2023

Choose a reason for hiding this comment

ThePedroo Jun 12, 2023

Choose a reason for hiding this comment

HackerSmacker left a comment

Choose a reason for hiding this comment

ThePedroo commented Feb 11, 2023

ThePedroo commented Feb 11, 2023 • edited Loading

ThePedroo commented May 15, 2023

HackerSmacker commented Jun 12, 2023

ThePedroo commented Jul 1, 2023

HackerSmacker left a comment

Choose a reason for hiding this comment

lcsmuller left a comment

Choose a reason for hiding this comment

Anotra commented Feb 2, 2024

ThePedroo commented Feb 2, 2024

ThePedroo commented Mar 21, 2024

lcsmuller commented Dec 7, 2024 • edited Loading

ThePedroo commented Feb 11, 2023 •

edited

Loading

lcsmuller commented Dec 7, 2024 •

edited

Loading