3

i have a method that do some api calling to save an image on filenet repository and after that it do some logging on two database tables, and the whole method takes lots of time to execute, so i thought of separating the logging into a separate thread as follows:

public void createNewDocument(){            
        doSaveImage();
        new Thread()
                    {
                       @Override
                       public void run() 
                       { 
                          try{ 
                              new LogDAO().addLog(log);
                              new ReportsDAO().addCreatedDocumentLog(productivityReport);
                          }catch(Exception e){
                             e.printStackTrace();
                          }
                       }
                    }.start();
    }

and it works fine with no issues for single user, so my question is that considerd a bad design and can it cause issues when multiple users are calling this method concurrently (memory issue ?) or safety issue ?

Mahmoud Saleh
  • 33,303
  • 119
  • 337
  • 498

6 Answers6

2

It's not a bad idea to open threads, caring about few things.

DB Connection opening/closing

Transaction will not span, each thread will have it's own transaction.

Rolling back the transaction, if any error/exception occurs in any worker thread

also how you manage your threads, are threads in runnable instance, or go >to pool, and instantiated everytime etc. talking about threadpoool here, >how it is managed.

Community
  • 1
  • 1
Ankur Singhal
  • 26,012
  • 16
  • 82
  • 116
2

Having many (hundreds of) threads at the same time is not a good idea.

First of all, your CPU only has maybe 4 or 8 cores, so it can't run more than that many threads at the same time anyway - it's not useful to have 100 threads ready to run at the same time, most of them will be waiting until a CPU core is free to run them.

Threads are relatively expensive to make. Each thread has a call stack. The default size of this call stack is platform-dependent but normally ranges from a few hundred KB to a few MB. That means that if you start 100 threads, just the call stacks of all those threads are already taking up lots of memory (up to hundreds of MBs).

You should use a thread pool instead. Java has a whole framework for thread pools in the java.util.concurrent package.

Example:

public void createNewDocument(ExecutorService executorService) {
    doSaveImage();
    executorService.execute(new Runnable() {
        @Override
        public void run() {
            try {
                new LogDAO().addLog(log);
                new ReportsDAO().addCreatedDocumentLog(productivityReport);
            } catch (Exception e) {
                e.printStackTrace();
            }
        }
    });
}

You create an ExecutorService using one of the factory methods in Executors, for example:

ExecutorService executorService = Executors.newFixedThreadPool(4);
Jesper
  • 202,709
  • 46
  • 318
  • 350
  • so in each call to create new document i will create a new ExecutorService ? – Mahmoud Saleh Mar 26 '15 at 12:42
  • also should i call at all the shutdown for the ExecutorService ? – Mahmoud Saleh Mar 26 '15 at 12:43
  • No, you should not create a new `ExecutorService` for every document. Create the `ExecutorService` once, and then re-use it. You need to call `shutdown()` or `shutdownNow()` on the `ExecutorService` when your program is stopping, or you don't need to create any new documents anymore. – Jesper Mar 26 '15 at 13:13
1

The main issue with having many threads is that changing the thread context means an overhead.

The crossing point were having more threads would cause prejudicial for your system will depend of how many requests do you have, and how much time each of those threads spend in an idle state (waiting for I/O or DB operations to happen, for instance).

The most safe approach would be creating a thread pool (which reuses threads, and is configured to have a maximum number of concurrent thread) and find the numbers that suit better to your system. Most servers that I know of will support this out of the box.

Additionally, your design has the drawback that you are swallowing any error that happens during operation. Your clients will think that the operation was successfully executed even if it was not. Think carefully if you can afford that.

SJuan76
  • 24,532
  • 6
  • 47
  • 87
1

As @ankur-singhal has pointed out there are other more-important factors when it comes to threading.

But, to answer your question about number of threads: A normal PC has about 1500 threads active on a normal day (across all applications and the OS itself). You can open Windows Task Manager and check this for yourself in the Performance tab.

This goes to show that normal PCs can handle thousands of threads easily.

On a server, if your application is the only one that is running, you could have 500-1000 threads without facing any issues.

However, just because you can does not mean you should!If you have a million requests per hour, you could have a thread pool of 500 threads. If you have 10000 requests per hour, you could have a thread pool of 25 or 50.

In any case, its a good design practice to use ExecutorService that plain threads.

And, don't forget to call shutdown when your application exits.

Teddy
  • 4,009
  • 2
  • 33
  • 55
1

You are proposing to create and destroy a new thread every time createNewDocument() is called. Creating and destroying a thread is more costly than waking up an existing, long-lived worker thread.

An instance of the java.util.concurrent.ThreadPoolExecutor class will manage a pool of worker threads for you. Each time your program wants to perform a task, it can give a Runnable object or a Callable object to the ThreadPoolExecutor, and the executor will wake up one of the pool threads which will perform the task.

If you don't call createNewDocument() all that often, then there's nothing wrong with way that you propose to do it, but it'll make you a better developer if you get in the habit of using the higher-level services (e.g., ThreadPoolExecutor) that the standard library and other libraries provide for you.

Solomon Slow
  • 25,130
  • 5
  • 37
  • 57
0

Because ,

i)It may easily lead to Deadlock state.

ii)Having too many threads will have an impact in the time complexity of the problem.

iii)Another problem arises when a time slice expires for a thread holding a lock. All threads waiting for the lock must now wait for the holding thread to get another time slice and release the lock. The problem is even worse if the lock implementation is fair, in which the lock is acquired in first-come first-served order. If a waiting thread is suspended, then all threads waiting behind it are blocked from acquiring the lock. It's like having someone fall asleep in a check-out line.