-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Same code starting wkhtmltopdf process works on netcoreapp3.1 but fails on net5.0 on Linux #46469
Comments
Tagging subscribers to this area: @eiriktsarpalis Issue DetailsDescriptionhttps://github.com/gurustron/repro-process-wkhtml-net-5. Next code starting process and reading stdout works fine in netcoreapp3.1:
but fails in net5.0 with:
Configuration
Regression?Works in netcoreapp3.1 Other informationrelated:
|
Following, facing the same issue. |
wkhtmltopdf, when instructed to output in stdout, tries to open the file
As far as I understand, there is no way to start a process from .NET 5 through |
so basically, as far as I understand, there is no current way to support wkhtmltopdf in .NET 5 in the current situation? That's a big thing for me, and as far as I believe for other developers that are exporting PDF's in their .NET applications. |
Agree with @mbashov's analysis. However the error in the linked reproduction is caused by the fact that it is disposing the Edit: fixed typo. |
@eiriktsarpalis changing code to: using (var memoryStream = new MemoryStream())
{
var baseStream = process.StandardOutput.BaseStream;
// using (var baseStream = process.StandardOutput.BaseStream)
//{
var buffer = new byte[4096];
int count;
while ((count = baseStream.Read(buffer, 0, buffer.Length)) > 0)
memoryStream.Write(buffer, 0, count);
var end = process.StandardError.ReadToEnd();
if (memoryStream.Length == 0L)
throw new Exception(end);
process.WaitForExit();
array = memoryStream.ToArray();
//}
} still throws the same error for me. And it works in .NET Core 3.1 without any changes. |
Apologies, my original comment should have said "StandardInput", namely making the following change: - using (var standardInput = process.StandardInput)
- {
- standardInput.WriteLine(html);
- }
+ process.StandardInput.WriteLine(html); In general, I would say there are too many |
@eiriktsarpalis thank you! this works for 5.0. Still wonder what actually changed since 3.1 cause it worked there with disposes =) Agreed on the disposes - code was written by someone else and I have not read it thoroughly enough. |
Great to know! In that case I'm going to close this issue. Thank you for your contribution. |
I was running wrong project. Removing the can you please reopen the issue? |
I was seeing the same, however the exact same behaviour happens when I call it with the same arguments directly from the shell, so I'm assuming this is the intended behaviour? |
works fine for me. The latest to switches (" - -") switch input to stdin and output to stdout. And again - 3.1 works with closing input, and sample in the docs closes the |
Could you please share your code which made it work? |
This is indeed the root cause for the issue. @stephentoub this is a regression caused by changing the |
That's unfortunate. How do other platforms deal with this? I've seen others use socketpair as well. Suggestions? It'd be a shame to need to revert. (Though if we can land #44647, maybe we could switch back to using pipes and use those types instead.) |
Other platforms using socketpair will run into the same problem. |
FWIW here's a minimal repro highlighting the regression: #include <stdio.h>
#include <fcntl.h>
int main()
{
if (open("/dev/stdin", O_RDONLY) < 0)
{
fprintf(stderr, "failed to open /dev/stdin.\n");
}
if (open("/dev/stdout", O_WRONLY) < 0)
{
fprintf(stderr, "failed to open /dev/stdout.\n");
}
} static void Main()
{
var process = new Process
{
StartInfo = new ProcessStartInfo
{
FileName = "<path to executable>",
RedirectStandardOutput = true,
RedirectStandardInput = true,
}
};
process.Start();
process.WaitForExit();
} The above prints the error messages in |
👋 I think I am also hitting this in the GitHub Actions runner, I updated the runner from 3.1 to 5.0.
What i see now is when the runner invoke a process, like
Is there any workaround I can try? or I will need to revert the runner back to 3.1. 😭 |
I can't think of a good workaround other than changing the code in the child process, which is not feasible in many cases. @stephentoub should we consider reverting #34861 for .NET 5? |
@tmds, why is this the case? That's expected? I see the docs for |
It's unfortunate, but if scenarios involving opening stdin/stdout/stderr via procfs are broken on Linux, we probably should (not a full revert, but a revert of the relevant pieces). If it works as-is on macOS, we could consider reverting just for Linux, too. The original PR was solving a real problem, though. Hopefully we could fix it again with #44647. |
POSIX When we changed to sockets, I expected this to be fine for the handles we provide in the child process. I hadn't considered this way of getting hold of the standard streams.
Someone with a mac can check using @eiriktsarpalis reproducer: #46469 (comment). |
Hi, Just wondering why Thanks, |
By "original issue", do you mean what #34861 was solving? Simply making the file descriptor non-blocking doesn't fix that. The hard part is all of the infrastructure necessary to enable efficiently performing the cancelable asynchronous I/O on top of non-blocking file descriptors via a mechanism like epoll on Linux or kqueues on macOS. There's already support for that under System.Net.Sockets.Socket, which is why #34861 changed Process to use socketpair, so that all of that infrastructure could be utilized. As an alternative, #44647 is working to change all of that infrastructure to support arbitrary file descriptors, at least for basic read and write operations, at which point it could be used with a pipe. |
@stephentoub Thank you for the explanation; maybe I need to go to sleep or at least step away from the computer - it didn't sink in when I read all of these reports. But, yup, using |
I confirm that the particular code doesn't reproduce on macOS. |
Fixed in #47461. |
* Add test validating against regression in dotnet#46469 * fix test bug
… (#47644) * Revert "Use socketpair to implement Process.Start redirection" (#47461) * Revert #34861 * reinstate removed test * Update src/libraries/System.Diagnostics.Process/tests/ProcessStreamReadTests.cs Co-authored-by: Stephen Toub <stoub@microsoft.com> Co-authored-by: Stephen Toub <stoub@microsoft.com> * Add test validating against regression in #46469 (#47643) * Add test validating against regression in #46469 * fix test bug Co-authored-by: Stephen Toub <stoub@microsoft.com>
Description
Repro.
Following code starting process and reading stdout works fine in netcoreapp3.1:
but fails in net5.0 with:
Configuration
5.0.101
Regression?
Works in netcoreapp3.1
Other information
related:
The text was updated successfully, but these errors were encountered: