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

773-sigill-introspection #793

Closed
wants to merge 10 commits into from

Conversation

tkornai
Copy link

@tkornai tkornai commented Jun 15, 2014

#773

This feature is intended to help analyse the state of gunicorn processes in a given point of time. This can come handy when dealing with performance issues.

A new signal handler for SIGILL would be introduced which triggers the status dump. When the signal is received workers dump their current requests with stack traces to the /tmp dir. If the signal is sent to the master it will broadcast it to its workers.

tempdir = util.gettempdir(self.cfg)
now = datetime.now()
time_stamp = now.strftime('%Y%m%d%H%M%S')
file_path = ("%s/gunicornsigill_%s_%s" % (tempdir, time_stamp, self.pid))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know we are using a POSIX system, but could you use os.path.join ? thanks

Copy link
Author

Choose a reason for hiding this comment

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

This is fixed, thanks for the hint.

@benoitc
Copy link
Owner

benoitc commented Jun 27, 2014

@tkornai I really like the idea. But by default gunicorn shouldn't rely on greenlets and use anything available on Python to do the job first. I understand why you're using the greenlets though.

Imo we should isolate the usage of greenlets for eventlet and gevent. (@tilgovi btw i don't hink we are using the async module for anything other than these two workers, maybe we should rename it?) . And find a way for the others workers based on asyncio and tornado. Would be cool if you can rework the patch in that direction :) Let me know if you have any question.

We can arrange a discussion on irc or other mean if needed, just let me know when since i am not so much about instant messaging ;)

@tkornai
Copy link
Author

tkornai commented Jun 30, 2014

Hi @benoitc - I've removed the greenlet dependency from sync and base. My feeling is that it's ok to move it to async as currently there is no worker based on async that does not require greenlet. I think introducing a new GreenletWorker at this point would be overengineering and can wait until such an async worker is added which does not depend on greenlet.

Unfortunately I'm not familiar with tornado so I don't really know how to tackle that case.

@tkornai
Copy link
Author

tkornai commented Jul 22, 2014

Hi @benoitc

I just wanted to pick up on this issue. Let me know if you need further changes on my side or what else would be required to push this further.

Thanks,
Tamas

@matrixise
Copy link
Collaborator

Thank you for the proposal and the patch, we need to discuss about some points, but we will come back to you with a response or a merged branch

@benoitc
Copy link
Owner

benoitc commented Jul 25, 2014

actually i wonder if we couldn't rename the worker async as gsync for greenlets sine async is mostly used for eventlet and gevent workers. (need to check with meinheld). Or maybe better to find a way to abstract such call so we could override it in gevent and eventlet workers. I am still undecided if it's OK to use the greenlet module directly in the async module. cc @tilgovi

@tilgovi
Copy link
Collaborator

tilgovi commented Jul 26, 2014

I only know about meinheld.

We could probably change it without too much disruption.

+1 for gsync or green or whatever you think it best.

@tilgovi
Copy link
Collaborator

tilgovi commented Jul 26, 2014

Only keep async if it can share code between greenlet and other asynchronous workers.

@benoitc
Copy link
Owner

benoitc commented May 2, 2016

closing since there are no activity since 2 years.

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

Successfully merging this pull request may close these issues.

None yet

6 participants