sdk-java icon indicating copy to clipboard operation
sdk-java copied to clipboard

Using executors incorrectly.

Open georgantasp opened this issue 7 years ago • 3 comments

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.

georgantasp avatar May 23 '17 16:05 georgantasp

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.

rahulrnitc avatar Dec 05 '17 11:12 rahulrnitc

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.

vijayabraj avatar Jun 21 '18 03:06 vijayabraj

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.

georgantasp avatar Jun 21 '18 13:06 georgantasp