move thread caller stack and rescue+log to a common place

[ColinDKelley]

I was happy to see this PR remove the "ThreadPool": https://github.com/guard/listen/pull/483 . It was a nice improvement to track the threads inside the listeners rather than globally.

All the threads should have a rescue-and-log at the top. And it's very handing to remember the original caller who created the thread to help identify it. That's done here: https://github.com/guard/listen/blob/master/lib/listen/adapter/base.rb#L72-L85

Let's put that in a general place and use it everywhere that threads are created. (And not raise from the top of the thread. That falls into some odd behavior in Ruby, where that exception can later be raised by code that calls join.)

Also: I don't believe the .kill is necessary here, since once a Thread#join returns, that means the thread has exited, right? https://github.com/guard/listen/blob/master/lib/listen/event/loop.rb#L64

[ColinDKelley]

@jonathanhefner Are you good with the changes I suggested here? If so, I'll make a PR.

1. Creating a wrapper around thread creation that keeps track of the thread creator and rescues/logs any exceptions, including that info.
2. Dropping .kill after thread.join. The Ruby Thread#join documentation says:

Does not return until thr exits or until the given limit seconds have passed.

And we're not passing limit seconds, so the part after or doesn't apply here.

[jonathanhefner]

@ColinDKelley I'm not actually affiliated with the Listen project, so I may not be the right person to ask. (I was fixing #476 to address some downstream issues in Rails.)

1. Creating a wrapper around thread creation that keeps track of the thread creator and rescues/logs any exceptions, including that info.

It might be good to ellaborate more about how this would be useful. From a cursory search, it looks like there are three places where Thread.new is called, and each has a rescue:

• Listen::Adapter::Base (the one you mentioned): Thread.new / rescue
• Listen::Adapter::Darwin: Thread.new / rescue
• Listen::Event::Loop: Thread.new / rescue

But perhaps these don't provide all the information you want when debugging? Or is this more about refactoring?

2. Dropping .kill after thread.join. The Ruby Thread#join documentation says:

Does not return until thr exits or until the given limit seconds have passed.

And we're not passing limit seconds, so the part after or doesn't apply here.

I think you're correct!

[ColinDKelley]

@jonathanhefner Ah, you're correct that each Thread.new does have a rescue. Now that I study them, I find there are 3 approaches to rescue and log--all of them different! I think it will be cleaner and more DRY to unify them.

Sorry I wasn't clear about the debugging information. One of the 3 Thread.new instances here keeps a copy of the caller who created the thread: calling_stack = caller.dup. I don't think the dup is necessary. But I do think keeping the caller is a great idea so that we can link any exceptions that get logged here to the caller who created that thread. Lacking that, it can be a real mystery sometimes.

And related, I believe the re-raise here is an anti-pattern: Once the exception is logged, it has been handled. Re-raising it will cause it to be stored by Ruby and raised again later when we call thread.join, which would be redundant at best. Although the comment says # for unit tests mostly, I didn't find any tests failing when I removed it.

I'll make a PR with these suggestions. And I'll drop the combinations of .kill and .join.

[ColinDKelley]

PR: https://github.com/guard/listen/pull/496

[ColinDKelley]

Fix merged into master for v3.3 milestone.