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

Using executors incorrectly. #115

Open
georgantasp opened this issue May 23, 2017 · 3 comments
Open

Using executors incorrectly. #115

georgantasp opened this issue May 23, 2017 · 3 comments
Labels

Comments

@georgantasp
Copy link

The SDK's HttpUtility is needlessly creating threads for every request execution.

        ExecutorService executor = Executors.newSingleThreadExecutor();
        Future<ANetApiResponse> future = executor.submit(new HttpCallTask(env, request, classType));
        executor.shutdown(); // Important!
		
        try {
        	response = future.get();
        	logger.debug(String.format("Response: '%s'", response));
	} ...

This is creating a new thread for every execution. EXPENSIVE!
I could be wrong, but I think it's a race condition where to assume the HTTPCallTask is started before the executor gets shutdown.
Waiting on future.get() which means no call is ever asynchronous.

@rahulrnitc
Copy link

Hi, thank you for your comments. This looks to be a valid issue and the team is actively looking into it. We will update this issue when a fix is in place.

@vijayabraj
Copy link
Contributor

Hi @georgantasp , the SDK's HttpUtility postData method is written that way for the following reasons:
a. New thread is created for every request execution so that it does not block other requests or become unresponsive.
b. Invoking the shutdown() will only initiate the shutdown process and it will execute all the tasks submitted to it before shutdown has been called. So that race condition will not occur.
c. future.get() has to be called to get the response from the future object and return it to the user immediately.

@georgantasp
Copy link
Author

Hi,

I'm pretty far removed from this concern at this point, but if I have time, I'll put together a PR to show you want I mean.

a. Creating new threads is expensive. The point of the executors package is to make it easy for developers to work with thread pools. In this way, a pool of 10 threads (for example) can be created and reused.
b. You're correct. docs
c. You are misunderstanding the point of threads. In a. you are saying that you don't want the calling thread to block, then in c. you are blocking the thread. If you want to be asynchronous, return the future itself and let the user decide if they want the thread to block.

I would encourage you to read "Java Concurrency In Practice" http://jcip.net/ . It's a great resource for learning more.

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

No branches or pull requests

4 participants