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

Add stdout and stderr callback support (Work in progress) #74

Closed
wants to merge 1 commit into from

Conversation

spinus
Copy link

@spinus spinus commented Sep 30, 2015

What do you think guys?

(there is still some issue with stderr but I'm not sure why, I'll look more at this)

@fizyk
Copy link
Member

fizyk commented Oct 4, 2015

Lack of error on correct output could be something with bash -c actually.

I'm not convinced about threads though, and to have them at the simple executor at that.

@spinus
Copy link
Author

spinus commented Oct 4, 2015

@fizyk - should I work more on that or cancel PR?
With asyncio or gevent I could use coroutines, not sure what I can use here instead of threads.

@fizyk
Copy link
Member

fizyk commented Oct 4, 2015

oh no, the general idea is great, just trying to wrap my head around it.

First things first, we really hadn't thought about piping stderr's before.

How about using poll objects? Similar to what we do in OutputExecutor?

If I understand this correctly, by using polling we'd get the most recent data when we'd ask, but by using threads, we'd pass all ouptut to callbacks, right?

@spinus
Copy link
Author

spinus commented Oct 4, 2015

Poll objects sounds good, I should check it before :-)
I'll try to rewrite that stuff in next weeks when I find some time.

@spinus
Copy link
Author

spinus commented Oct 6, 2015

OK, I looked at that more and I don't see how to use that without main loop. You used select.poll for waiting so in that moment you control the program flow. I could use the same method to make logs available for the executor user and he could retrieve them whenever he want, but this can cause buffer problems when output is bigger. Nice solution would be to fetch output regularly, but to do that thread, main loop, process or whatever else is needed. Please let me know if you see other way so solve this and what should I do with this code.

@fizyk
Copy link
Member

fizyk commented Oct 7, 2015

@spinus thanks for analysis. Haven't actually thought of that.

My first concern with current solution is OutputExecutor, threads might interfere with poll. Won't output will be either read by callback or by wait method?

I see two possible solutions:

  • a separate method to set callbacks - I don't think setting callbacks within start is a good solution, there's a bit happening in there already
  • or - second approach - a kind of MixIn, where dev could Mix callbacks in a subclassed executor - given that the mixin would fit in without any more effort on dev side.

What's the actual use case you're pursuing?
I actually haven't thought about reading subprocess output constantly.

@spinus
Copy link
Author

spinus commented Oct 7, 2015

Use case is:
I'm using mirakuru to start server processes for tests, I'm using pytest.
I would like to catch output (stdout,stderr) from servers I'm testing using python logging, sometimes lot of lines.

setting callbacks can be done as properties as well, all other implementation stays the same.

OutputExecutor can be tricky, sure. If we decide to implement that stuff I'll make sure OutputExecutor is working as expected.

@fizyk
Copy link
Member

fizyk commented Oct 8, 2015

Okay, we used to check log files when we needed, but this approach is good as well. Question, will you need access to log files from start or from certain point?

Let's add them as separate properties to be able to set at first, though thinking about that the whole process could be packaged into it's own class - kind of ProcessFactory - providing output either by poll, by thread callbacks (later gevent, coroutines for py35 (?), default factory will be packaging the basis as it is now, and it's subclasses will provide additional functionalities.

But let's first get the thread callbacks working.

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.

None yet

2 participants