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

Make running tokei thread-safe #42

Merged
merged 1 commit into from
Mar 21, 2021
Merged

Make running tokei thread-safe #42

merged 1 commit into from
Mar 21, 2021

Conversation

giordano
Copy link
Member

No description provided.

@ericphanson
Copy link
Member

ericphanson commented Mar 21, 2021

interesting, I see in the JLLWrappers source it says this is the "new-style addenv()-based function". Why is that one threadsafe and not the setenv version?

edit: nevermind, found JuliaLang/julia#37070

@giordano
Copy link
Member Author

See also JuliaLang/julia#34726 for setenv not being thread-safe.

I believe this is what was causing all my troubles with cloning repositories today when using multiple threads. With this fix, I don't have any issues. Luckily we already require Julia v1.6.

@ericphanson
Copy link
Member

ericphanson commented Mar 21, 2021

Sounds good!

Yeah, I think I didn't really realize that withenv just mutated the global environmental variables, ran the command, and changed them back (which sounds very thread-unsafe). I kind of assumed it was doing something smarter like have some kind of local environment that the command runs in, which I guess is what the new way does.

@giordano giordano merged commit fc4c911 into main Mar 21, 2021
@giordano giordano deleted the mg/tokei-thread-safe branch March 21, 2021 19:40
@ericphanson
Copy link
Member

I went to check LicenseCheck.jl to see if I use the setenv style commands, and it turns out I have a direct ccall: https://github.com/ericphanson/LicenseCheck.jl/blob/2c22cb241aceb8755c673174c9e198c37011e3c6/src/LicenseCheck.jl#L34-L37. Do you think that's OK?

@giordano
Copy link
Member Author

The problem is only for the executables (they need to have some environment variables to be set), not libraries

@ericphanson
Copy link
Member

I guess there could be an issue if the library couldn't handle being called into by multiple threads (if it was mutating its own globals or something when I call one of its functions), but we haven't seen that yet for licensecheck so it's probably OK.

@ericphanson
Copy link
Member

we do cd a few lines above-- could that be a threading issue too? If one thread changes the directory to A, then before tokei runs, another thread changes it to directory B, then maybe we'll count lines in the wrong directory?

@ericphanson
Copy link
Member

btw, I imagine 90%+ of the time is spent on IO, so we could probably just use asyncmap instead of a threaded loop and not need to worry about thread-safety? though it is nice to figure out the right way to do these things in a threaded context

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.

2 participants