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

Fixed output of logs (tail/maintail) in the Web UI #195

Closed
wants to merge 1 commit into from

Conversation

adw0rd
Copy link

@adw0rd adw0rd commented Jan 3, 2013

Because forgot to give the contents via tail_f_producer.more()

… give the contents via tail_f_producer.more()
@mnaberez
Copy link
Member

mnaberez commented Jan 4, 2013

Could you please let me know the issue that this patch fixes, and how to check it?

@adw0rd
Copy link
Author

adw0rd commented Jan 4, 2013

Hello,

I run the Web UI (inet_http_server), click to any "Tail -f" of processes, and the page begins to endlessly try to connect, followed by a 504 error web-server.

I look at the source code, and thought it simply forgot to call .more() for class tail_f_producer, because the class itself does not return to __init__() (https://github.com/adw0rd/supervisor/blob/master/supervisor/http.py#L644). From this I concluded that it is necessary to add tail_f_producer.more().

And more, I think that we should instead literal 1024:

request.push (tail_f_producer (request, logfile, 1024). more ())

pass this argument, which must be defined in the configuration file.

@nanonyme
Copy link

Could this please be reviewed and either accepted or rejected asap? Blocks: #149

@qfox
Copy link

qfox commented Jul 12, 2014

+1 to review and accept this.

@antonbabenko
Copy link

Please merge this.

@mnaberez
Copy link
Member

I don't think this change is correct because it breaks supervisorctl tail -f.

Python script spew.py that writes some text to stdout every 100ms:

import string, sys, time

while True:
    sys.stdout.write(string.digits)
    sys.stdout.flush()
    time.sleep(0.1)

Minimal supervisord.conf to run it:

[supervisord]
logfile = /tmp/supervisord.log

[inet_http_server]
port = 127.0.0.1:9001

[supervisorctl]
serverurl = http://127.0.0.1:9001

[rpcinterface:supervisor]
supervisor.rpcinterface_factory = supervisor.rpcinterface:make_main_rpcinterface

[program:spew]
command = python /path/to/spew.py

Running supervisord in the foreground with that config:

$ supervisord -n -c /path/to/supervisord.conf

Tailing the log with supervisorctl tail -f:

$ supervisorctl -c /path/to/supervisord.conf tail -f spew

Before this patch, supervisorctl tail -f works as expected and streams the output indefinitely. After this patch, it shows some output and then stops.

@ngocson2vn
Copy link
Contributor

Please consider my fix #471

@@ -733,7 +733,7 @@ def handle_request(self, request):
# the lack of a Content-Length header makes the outputter
# send a 'Transfer-Encoding: chunked' response

request.push(tail_f_producer(request, logfile, 1024))
request.push(tail_f_producer(request, logfile, 1024).more())
Copy link
Member

Choose a reason for hiding this comment

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

The original code pushed the tail_f_producer instance onto the request. This was intentional because the code is asynchronous. Once the producer instance is pushed, the event loop will call more() on it each time it comes around. This is how we are able to stream log output continuously when you use supervisorctl tail -f.

This patch changed the code to push the result of tail_f_producer(x).more() instead of the producer instance. The result of more() will be a string with some log output. This is why the patch may have appeared to fix the output on the web interface. However, this doesn't set up the producer in the event loop like we originally intended, and it can never stream this way (the -f in tail -f).

@mnaberez
Copy link
Member

I'm closing this pull request for the reasons noted above. The original issue (#149) is still open and there are now two pull requests with different solutions (#444, #471) that need to be reviewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

6 participants