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

[Issue #218] StdOut was not being redirected properly and was causing… #220

Merged
merged 1 commit into from
Jun 5, 2017
Merged

[Issue #218] StdOut was not being redirected properly and was causing… #220

merged 1 commit into from
Jun 5, 2017

Conversation

pnikonowicz
Copy link

@pnikonowicz pnikonowicz commented May 17, 2017

… the child process to hang.

When this is merged, we should create a new task to include logging of stdout properly using the Log4Net feature that was recently completed.

Edited by @NextTurn: Fixes #218

@pnikonowicz
Copy link
Author

build is failing because .Net 4+ features were used and are incompatible with .Net 2.0.
Will fix soon.

@pnikonowicz
Copy link
Author

build should be fixed now. if so, please merge.

@oleg-nenashev
Copy link
Member

IIRC the plan in #218 was to actually make it working. OTOH it can be applicable as a hotfix. I will try to test and merge something this week. Was traveling, so missed the PR.

Please also revert the permission change in the file. There is no reason for these files to be executable

@pnikonowicz
Copy link
Author

Yes, the idea was to do a hot fix on the main problem, and then create a new task for the remaining work. I tried to explain that in the PR description, sorry if that wasn't clear.

I've been busy this week but will address the file perms soon.

ps.RedirectStandardInput = true; // this creates a pipe for stdin to the new process, instead of having it inherit our stdin.
ps.RedirectStandardOutput = true;
ps.RedirectStandardError = true;
ps.RedirectStandardInput = false; // this creates a pipe for stdin to the new process, instead of having it inherit our stdin.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd guess this comment is also outdated now

@pnikonowicz
Copy link
Author

file perms and comment issues have been fixed.

@pnikonowicz
Copy link
Author

I would like to work on some more items but would like to see this PR closed or merged before that happens.

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Jun 5, 2017 via email

@oleg-nenashev oleg-nenashev self-assigned this Jun 5, 2017
@oleg-nenashev oleg-nenashev merged commit 10c1ec3 into winsw:master Jun 5, 2017
oleg-nenashev added a commit to oleg-nenashev/winsw that referenced this pull request Jun 8, 2017
…ld redirect STDOUT/STDERR when LogHandler is defined

It restores logging of executables, which has been broken in winsw#220.
Not a regression, because the change has not been released yet
@nxtn nxtn modified the milestones: 2.2.0, 2.1.1 Mar 25, 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.

writing to stdout from a stopexecutable will eventually cause the process to hang
3 participants