Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

create pthreads as detached instead of joinable #52

Merged
merged 1 commit into from
Apr 6, 2022

Conversation

fralken
Copy link
Collaborator

@fralken fralken commented Apr 3, 2022

Threads can be created as detached instead of joinable because we don't need to get their return value when they terminate.
Socks5 threads did not put themselves in the list of joinable threads so they were never joined anyway.

Now detached threads put themselves in the list of terminated threads only for monitoring, so that cntlm when receives a sigterm signal can decide to exit or waiting for pending thread ti finish.

The serialize flag ("-s", only used for debugging) was used only for proxy_thread but not for socks5_thread and tunnel_thread. Now it is checked on all cases.

This PR fixes #49, because pthread_join() must not be called on detached threads.
Maybe it can be useful for #18 (@jschwartzenberg can you do some tests?) and maybe also for #3.

I tested both proxy and socks5 by executing one thousand concurrent curl requests, and for me it worked flawlessly.

@sonarcloud
Copy link

sonarcloud bot commented Apr 3, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@fralken
Copy link
Collaborator Author

fralken commented Apr 3, 2022

Since we don't need the threads' ids (because we do not join them) we don't need to fill the plist, but just a counter of terminated threads is enough.
This logic can be further simplified then.

@versat
Copy link
Owner

versat commented Apr 4, 2022

Looks good to me, this is way cleaner now.
I am just curious: Have you tested the 1000 concurrent curl requests also with an older version? Maybe with the original one (old Cygwin libs) where the pthread join worked?

@fralken
Copy link
Collaborator Author

fralken commented Apr 5, 2022

Yes, I tested the old version, too. And it works. I'm still using the old 0.92.3 in my current Windows configuration, and it works.
Actually I noticed that the EINVAL error in that specific scenario appears to be a false positive, since there are no leaks of thread resources even under stress tests.
Anyway I noticed that there is no need to join threads when we don't need their return value, it is simpler to start them as detached. It is still useful to keep the logic that counts the finished threads, so that when it receives a sigterm it can decide to wait for the still active threads to finish.

@versat versat merged commit a69c6f5 into versat:master Apr 6, 2022
@fralken fralken deleted the pthread branch April 7, 2022 07:52
@jschwartzenberg
Copy link
Collaborator

I can confirm this solves the crash issue I had! Great!! Thanks a lot!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The Cygwin version of cntlm fails pthread_join()
3 participants