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

Does execution.isolation.strategy really default to THREAD as stated in the docs? #1383

Closed
dagronnell opened this issue Oct 14, 2016 · 8 comments

Comments

@dagronnell
Copy link

dagronnell commented Oct 14, 2016

Running a test with javanica annotations and it seems like default execution isolation strategy is SEMAPHORE and not THREAD. Is this a bug or error in the docs or did I misunderstand something?

The test below will print:

Command thread-id: 1
Main thread-id: 1

Uncommenting the @HystrixProperty will make the test print out different thread-ids.

@Test
public void name() throws InterruptedException {
    Observable<String> result = annotatedService.hello();
    System.out.println("Main thread-id: "+Thread.currentThread().getId());
    result.subscribe(
            System.out::println,
            Throwable::printStackTrace
    );
    Thread.sleep(5000);
}

public class AnnotatedService {
    @HystrixCommand(
            commandProperties = {
//                    @HystrixProperty(name = "execution.isolation.strategy", value = "THREAD")
            }
    )
    Observable<String> hello() {
        System.out.println("Command thread-id: "+Thread.currentThread().getId());

        return Observable.create(subscriber -> {
            if (!subscriber.isUnsubscribed()) {
                try {
                    Thread.sleep(5000);
                    subscriber.onNext("Hello");
                    subscriber.onCompleted();
                } catch (InterruptedException e) {
                    throw new RuntimeException(e);
                }
            }
        });
    }
}
@56quarters
Copy link

FWIW, I noticed the same thing. Seems to only be with methods that return Observable<T>. Methods that return Future<T> and T use threads for isolation by default.

@mattrjacobs
Copy link
Contributor

mattrjacobs commented Oct 14, 2016

When not using Javanica, HystrixCommand instances default to using thread-isolation and HystrixObservableCommand instance default to using semaphore-isolation.

Javanica makes this a bit more implicit by using @HystrixCommand for both. As @56quarters mentioned, the return type of the method determines if a HystrixCommand or HystrixObservableCommand gets created.

@dagronnell
Copy link
Author

Thanks for the clarification!

What's the reason behind HystrixObservableCommand defaulting to using semaphore-isolation?

@mattrjacobs
Copy link
Contributor

See https://github.com/Netflix/Hystrix/wiki/FAQ%20:%20Operational. If you are using HystrixObservableCommand, the assumption is that the work you're wrapping with Hystrix is already async. If that's the case, then using a threadpool doesn't get you anything, therefore it is not supported.

@dmgcodevil
Copy link
Contributor

based on this conversation can we say that there is no issue and everything is working as expected in this regard ?

@ghostganz
Copy link

If you read https://github.com/Netflix/Hystrix/wiki/Configuration you get the impression that THREAD is always the default:

The default, and the recommended setting, is to run commands using thread isolation (THREAD).

There could at least be a footnote about it sometimes not being the default there. We thought we were on THREAD until we were surprised to hit a semaphore limit.

@mattrjacobs
Copy link
Contributor

Thanks for the report, @ghostganz . I just modified https://github.com/Netflix/Hystrix/wiki/Configuration#thread-or-semaphore. Any further changes that may be helpful?

@geekthanos
Copy link

geekthanos commented Sep 14, 2018

@mattrjacobs So if I am using hystrix observable which uses semaphore, does that mean I cannot update the timeout? Will it be defaulted to 1000ms always?

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

No branches or pull requests

6 participants