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

Configure threads to not abort on exceptions #484

Merged
merged 5 commits into from
Jul 1, 2016
Merged

Configure threads to not abort on exceptions #484

merged 5 commits into from
Jul 1, 2016

Conversation

ayufan
Copy link
Contributor

@ayufan ayufan commented Jun 20, 2016

This solves long outstanding issue: #396.

This happens, because some applications or gems do overwrite global setting of Thread.abort_on_exception= which is by default set to false.
This leads to application process to exit and making the spring unable to recover from that.

Examples of such:

This solves long outstanding issue: #396.
This happens, because some applications or gems do overwrite global setting of `Thread.abort_on_exception=` which is by default set to `false`.
This leads to application process to exit and making the spring unable to recover from that.
@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @jonleighton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@ayufan ayufan mentioned this pull request Jun 21, 2016
lework pushed a commit to lework/gitlab-ce that referenced this pull request Jun 22, 2016
@jonleighton
Copy link
Member

This seems good, thanks. Some comments:

  • Please add to the changelog file
  • I suggest extracting all the repetitive code into a Spring.failsafe_thread { ... } method

@ayufan
Copy link
Contributor Author

ayufan commented Jun 30, 2016

Updated. Can you review @jonleighton?

@tycooon
Copy link

tycooon commented Jun 30, 2016

I tried this fork in my app (see #466), but no luck. Actually, this version just doesn't work: it tries to reload app right after i run rspec (even though i didn't change any files) and hangs the same way it does in original version.

@ayufan
Copy link
Contributor Author

ayufan commented Jun 30, 2016

It seems strange, because your problem seems very similar to mine. Can you post maybe more logs? Also can you post what gems are you using?

@tycooon
Copy link

tycooon commented Jun 30, 2016

Here is the log: https://gist.github.com/tycooon/61a711940539643bb17b9151a4cffe7d
I stopped spring and then started/closed rails console several times (it hanged on the 3rd attempt).

@ayufan
Copy link
Contributor Author

ayufan commented Jun 30, 2016

@tycooon Are you sure that you are using my changes?

@tycooon
Copy link

tycooon commented Jun 30, 2016

Yes, I've put gem "spring", github: "ayufan/spring" in my Gemfile and run bundle. And spring's behavior actually changed, it doesn't work.

@jonleighton
Copy link
Member

Yes, I've put gem "spring", github: "ayufan/spring" in my Gemfile and run bundle. And spring's behavior actually changed, it doesn't work.

This is not a supported way of using spring. If you want to test this branch, you'll need to check it out and build and install the gem manually.

@tycooon
Copy link

tycooon commented Jul 1, 2016

OK, now I made these steps:

git clone https://github.com/ayufan/spring
cd spring && gem build spring.gemspec
gem uninstall spring
gem install --local ./spring-1.7.1.gem

Now it doesn't die when I run rails c several times but still hangs after trying to reload app after touch Gemfile (the same behavior as original 1.7.1 version). Here is the log: https://gist.github.com/tycooon/aa8cba7947563d52bb113304216ef38c

@ayufan
Copy link
Contributor Author

ayufan commented Jul 1, 2016

@tycooon Strange. It works without any problems for me now :)

Maybe you can post your gems?

@jonleighton jonleighton merged commit 7100ae4 into rails:master Jul 1, 2016
@jonleighton
Copy link
Member

I've released 1.7.2 with this fix, so you can try that. Thanks!

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.

4 participants