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

Implement Sync for process::Command #72387

Closed
jonhoo opened this issue May 20, 2020 · 2 comments · Fixed by #72472
Closed

Implement Sync for process::Command #72387

jonhoo opened this issue May 20, 2020 · 2 comments · Fixed by #72472
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@jonhoo
Copy link
Contributor

jonhoo commented May 20, 2020

std::process::Command is currently Send, but not Sync. While Sync probably isn't super useful for Command itself, it does mean that any type that wraps a process::Command also isn't Sync even if it might be useful on that type.

Command currently has a manual impl Send following #47760 (issue #47751), though from the discussion on users.r-l.o, it sounds like a manual impl Sync would also be sound. @cuviper not sure if that still holds?

If it does, this seems like a worthwhile impl to add, simply for the auto-trait implications.

@jonhoo jonhoo added the C-bug Category: This is a bug. label May 20, 2020
@jonhoo
Copy link
Contributor Author

jonhoo commented May 20, 2020

To give one example of where this crops up: tokio::process::Command currently isn't Sync because std::process::Command isn't Sync. This also means that any async fn that uses tokio::process::Command across an await point is not Sync. Again, the ramifications of that probably aren't too severe (how important is it that a Future is Sync?), but it does lead to some annoyance when trying to use that future with libraries that just stick Send + Sync bounds wherever to get rid of compiler errors 😅

@cuviper
Copy link
Member

cuviper commented May 20, 2020

As with Send, I think it's just the argv that isn't Sync, but that should be fine to add.

@jonas-schievink jonas-schievink added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. C-enhancement Category: An issue proposing an enhancement or a PR with one. and removed C-bug Category: This is a bug. labels May 20, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 25, 2020
…lnay

Implement `Sync` for `process::Command on unix and vxworks

Closes rust-lang#72387.
r? @cuviper
@bors bors closed this as completed in 2679c38 May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants