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

WIP Add :directory parameter to ccl:run-program #337

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jdz
Copy link
Contributor

@jdz jdz commented Aug 13, 2020

No description provided.

@phoe
Copy link
Contributor

phoe commented Aug 13, 2020

Will this function work with multiple threads without race conditions? We are accessing CWD, which is a process-global singular resource, so multiple threads which launch programs simultaneously may collide with each other on attempting to set this.

@jdz
Copy link
Contributor Author

jdz commented Aug 13, 2020

Will this function work with multiple threads without race conditions?

The directory is changed in the child process after fork (right before executing the external program), and I'm pretty sure the issues you mention are not relevant. Am I missing something?

@phoe
Copy link
Contributor

phoe commented Aug 13, 2020

OK - if we have already forked, then we are mutating the new process's CWD, and so the old process's CWD is untouched. Thanks for clarification.

@jdz
Copy link
Contributor Author

jdz commented Aug 13, 2020

One thing I'm wondering is whether the external command should be executed if chdir fails? I think not, so more work needs to be done here.

@xach
Copy link

xach commented Aug 13, 2020 via email

@jdz
Copy link
Contributor Author

jdz commented Aug 13, 2020

My (not very original) plan is to create a pipe and communicate any failures to parent process through that.

@jdz jdz changed the title Add :directory parameter to ccl:run-program WIP Add :directory parameter to ccl:run-program Aug 13, 2020
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.

3 participants