-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
GAE prohibits spawning new threads directly #795
Comments
(copied from #734 (comment)) Originally written by @chrisbianca: @mattrjacobs Sadly, whilst this has removed Unsafe as a stumbling block on GAE, it fails at the next hurdle instead which is the spawning of new Threads:
GAE restricts the spawning of new threads other than through it's ThreadManager (https://cloud.google.com/appengine/docs/java/javadoc/com/google/appengine/api/ThreadManager). To work around this, the creation of new threads in Hystrix would have to make use of a ThreadFactory instead of creating new Threads directly. Do you think this might be possible? There's a good example in Guava of how they've worked around the limitation on GAE: http://docs.guava-libraries.googlecode.com/git/javadoc/src-html/com/google/common/util/concurrent/MoreExecutors.html#line.766 |
(copied from #734 (comment)) (originally written by @chrisbianca) @mattrjacobs : I had a bit of a play round with this myself. Unfortunately whilst using the ThreadFactory does alleviate the error, it means that you're not able to name the threads as they are currently: Thread.setName() is also a restricted GAE class. Is there a reason the threads have to be named hystrix-XXX? |
@mattrjacobs : Removing setName() and setDaemon() means that GAE will actually execute the HystrixCommand without any errors. I've not studied the Hystrix code enough to establish what knock on effects this might have on functionality though. |
AFAIK, changing names is a change that humans would notice but the Hystrix runtime wouldn't care about. I'll need to review the code to confirm this. setDaemon was a fairly recent change, so not performing that in GAE would be OK. This only affects behavior at shutdown. If those changes would be reasonable, then I would likely gate them to change the behavior for GAE, and leave all other users as-is. The implementation from Guava looks right (and I'm sure Google knows better than I do about how to detect AppEngine) |
Yes, that seems like a reasonable approach, and certainly hacking in those changes on a local build has allowed me to at least make use of the core Hystrix functionality on GAE. I haven't had a chance to check any of the other functionality, e.g. servlet, as yet. I'll try and do that next week, but this will certainly be a good step in the right direction. |
For reference, the places in code which call newThread currently are:
|
@chrisbianca Could you take a look at #1066? |
It seems that we are seeing the same issue. Do these changes also apply in Google app engine flex/compat runtime? |
You'll have to forgive me - I know nothing about GAE. The changed outlined above were:
2 was never tested If you could report what version of Hystrix you're using and what error you're encountering, that would help understand what the next step is |
Issue found after #734 change
The text was updated successfully, but these errors were encountered: