0
\$\begingroup\$

I have server-client communication architecture where there is one server and 150 clients.

The server-client communication happens via Java NIO where all the clients send some or the other data every 10 seconds. Previously we used to queue all the process messages and process all those in a single thread, as the number of clients are more so as the messages, server is not able to process all the messages instantly and there is a delay in processing in turn data loss. So I have thought of implementing CachecThreadPool to process the tasks simultaneously as soon as they come, picked CachedThreadPool over FixedThreadPool because the tasks are short lived and many in number, below is the code for that. The thread which receives messages from client calls ProcessorClass.processData(message) as soon as it receives the message.

public class ProcessorClass{

    private static final Logger LOGGER = Logger.getLogger(ProcessorClass.class);
    static ThreadPoolExecutor executor = (ThreadPoolExecutor) Executors.newCachedThreadPool();

    public static void processData(StringBuffer message) {
        Runnable task = new Runnable() {
            @Override
            public void run() {
                try {
                    LOGGER.info("Queue size:"+executor.getQueue().size());
                    if (message != null){
                        processMessage(message);
                    }
                }
                catch(Exception e) {
                    LOGGER.error("Error happened in run() method" + e.getMessage());
                }

            }
        };
        executor.execute(task);
    }
    public static void processMessage(StringBuffer message){
        // all the processing of message such as DB operations goes here.
    }
}

Doubts:

  1. How CachedThreadPool stores the message in the queue because i haven't defined any explicitly.
  2. Should i chose FixedThreadPool over this?
  3. Should i make my processMessage() method synchronized?

All the suggestions and review comments are welcome.

\$\endgroup\$

closed as off-topic by vnp, Quill, 200_success, Grajdeanu Alex., t3chb0t Nov 25 '18 at 18:31

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – vnp, Quill, 200_success, Grajdeanu Alex., t3chb0t
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ Your code is missing something. What's inside processMessage? Is it thread-safe? More context usually leads to a better question and better answers. \$\endgroup\$ – Mast Nov 22 '18 at 14:31
  • \$\begingroup\$ inside processMessage i am performing database operations by parsing the message received,as far as i know i am not doing any operations which deals with shared objects inside that method. \$\endgroup\$ – raviraja Nov 22 '18 at 14:44
1
\$\begingroup\$
  1. The thread pool is not storing messages on a queue; it is storing Runnable tasks. However, each Runnable task has access to the effectively final StringBuffer message variable from the calling context.

  2. See this StackOverflow question. Each is a ThreadPoolExecutor, just with different parameters to control how many threads are created and the type of queue. I would lean in favour of the FixedThreadPool with 1 thread for each CPU core, but many other factors can and will impact that; profile to be sure.

  3. No. Making processMessage() synchronized will mean you are single threaded ... worse, because the overhead of multiple threads being scheduled to process the task queue in a single threaded fashion. At 150 messages/10 seconds, with one thread the function would need to complete in 66ms. If you have 8 cores, with 1 thread per core, each thread could take a leisurely 500ms to process the message, as long as they can run independently. You may have parts of processMessage that you may need to make synchronized, but you’ll want to make those regions as small and fast as possible.

Other comments:

A StringBuffer is a mutable object; since message is being passed to another thread, for safety, I’d want the message to be transformed into a immutable String so the caller doesn’t clear the message buffer while the executor thread is processing it.

processMessage() does not need to be public; private would be more appropriate.

It looks like executor should be private and final as well.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.