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

Perform a VM-wide thread stack trace dump on USR2 #263

Merged

Conversation

olleolleolle
Copy link
Contributor

@olleolleolle olleolleolle commented Nov 18, 2016

This is a PR inspired by another project which had a neat section of signals to send to the worker, to have to do things, like "shed current work item, never to requeue it" or "print a debug backtrace".

Inspiration

Questions

  • What signals should the Worker process handle?
  • Can you golf the control structure exception away? (See - this is me feeling safe enough here to publish some not-so-great code.)

Proposed changes

  • introduce a USR2 signal to print all the Threads' backtraces
  • generally keep the signals uppercased


sig = sig_read.gets.strip.downcase
if USER_SIGNALS.include?(sig.upcase)
logger.info "FUN TIME! #{sig}"
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate the humour but perhaps we should make this log message a bit more descriptive :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I should have written "This is a handler for signal SIGUSR1, the behavior of which we will define as a community".

The current implementation is only a skeleton logging "proof" that it works.

@michaelklishin
Copy link
Member

Some common signals are TERM and INT, HUP (e.g. for config reloading — I don't think Hutch needs this, tbh), USR1 and USR2 (meaning entirely user defined).

If you don't have any specific ideas about what Hutch would do when sent a USR1 or USR2,
I'm not sure we should trap them at all.

@michaelklishin michaelklishin changed the title Waiter: add USER_SIGNALS USR1, USR2 DO NOT MERGE Proof of concept: add USR1, USR2 signal handling to Waiter Nov 18, 2016
@eebs
Copy link

eebs commented Nov 18, 2016

It might be useful to allow pausing/resuming of the consumers. I see that Bunny could support pausing the work pool, but I didn't see anything in March Hare after a 2 minute glance.

@michaelklishin
Copy link
Member

The only way to pause a consumer is to stop deliveries to it by tweaking QoS prefetch. Otherwise you will run out of memory quickly. This sounds like a dangerous, hard to get right idea to me.

@eebs
Copy link

eebs commented Nov 18, 2016

Ah okay sorry, I was looking at Bunny's ConsumerWorkPool#pause and Channel#maybe_pause_consumer_work_pool!, but it looks like those aren't actually used anywhere.

edit Looking closer I now see that things would still be pushed on to the queue, they just wouldn't be processed.

@olleolleolle olleolleolle changed the title DO NOT MERGE Proof of concept: add USR1, USR2 signal handling to Waiter DO NOT MERGE Proof of concept: add USR2 signal handler to Waiter Nov 20, 2016
@olleolleolle
Copy link
Contributor Author

I rebased.

@michaelklishin
Copy link
Member

So USR2 performs a thread dump. I like this idea. Do we leave the door open for USR1 handling for now then?

@michaelklishin michaelklishin self-assigned this Nov 20, 2016
end

private

def handle_usr2
Thread.list.each do |thread|
Copy link
Member

Choose a reason for hiding this comment

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

I suggest that we log a header such as "requested a VM-wide thread stack trace dump…"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed!

end

private

def handle_usr2
Thread.list.each do |thread|
logger.warn "Thread TID-#{thread.object_id.to_s(36)} #{thread['label']}"
Copy link
Member

Choose a reason for hiding this comment

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

Why warn and not info?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be info!

@michaelklishin michaelklishin changed the title DO NOT MERGE Proof of concept: add USR2 signal handler to Waiter Perform a VM-wide thread stack trace dump on USR2 Nov 20, 2016
@michaelklishin michaelklishin merged commit 1c3658a into ruby-amqp:master Nov 20, 2016
@olleolleolle olleolleolle deleted the feature/add-more-signals branch November 20, 2016 18:56
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.

3 participants