3

Note: I understand the rules site, but I can't to put all code (complex/large code).

I put a DIFFERENT (all the real code is too much and you don't need here) code in Github but reproduces the Problem (the main class is joseluisbz.mock.support.TestOptimalDSP and switching class is joseluisbz.mock.support.runnable.ProcessorDSP) like the video.

Please don't recommend to me another jar or external library for this code.

I wish I was more specific, but I don't know what part to extract and show. Before you close this question: Obviously, I am willing to refine my question if someone tells me where to look (technical detail).

I made a video in order to show my issue.

Even to formulate the question, I made a diagram to show the situation.

My program has a JTree, showing the relations between Worker.

enter image description here

I have a diagram interaction between threads controlling life with ExecutorService executorService = Executors.newCachedThreadPool(); and List<Future<?>> listFuture = Collections.synchronizedList(new ArrayList<>());

Each Runnable is started in this way listFuture().add(executorService().submit(this)); in its constructor. The lists are created like this: BlockingQueue<Custom> someBlockingQueue = new LinkedBlockingQueue<>();

enter image description here

My diagram shows who the Worker's father is if he has one. It also shows, the writing relationships between the BlockingQueue.

RunnableStopper stops related runnables contained in Worker like property. RunnableDecrementer, RunnableIncrementer, RunnableFilter operates with a cycle that runs each Custom that it receives for its BlockingQueue. For which they always create a RunnableProcessor (it has no loop, but because of its long processing, once the task is finished it should be collected by the GC).

Internally the RunnableIncrementer has a Map Map<Integer, List<Custom>> mapListDelayedCustom = new HashMap<>();//Collections.synchronizedMap(new HashMap<>());

When arrives some Custom... I need to obtain the List of lastReceivedCustom List<Custom> listDelayedCustom = mapListDelayedCustom.putIfAbsent(custom.getCode(), new ArrayList<>()); I'm controlling the Size (is not growing indefinitely).

My code stops working when I add the following lines:

if (listDelayedCustom.size() > SomeValue) {
  //No operation has yet been included in if sentence
}

But commenting the lines doesn't block

//if (listDelayedCustom.size() > SomeValue) {
//  //No operation has yet been included in if sentence
//}

enter image description here What could be blocking my Runnable? It makes no sense that adding the lines indicated (Evaluate the size of a list: if sentence) above stops working.

Any advice to further specify my question?

joseluisbz
  • 1,491
  • 1
  • 36
  • 58
  • The context of your question isn't really clear to me. Would you be able to create an [MCVE](https://stackoverflow.com/help/minimal-reproducible-example)? – Jacob G. Dec 12 '19 at 02:41
  • @JacobG. Now, I'm uploading a video to show the problem because I don't know the cause, I don't know what code extract in order to repeat the problem ant put here. https://youtu.be/KXLcR-N8qcY (I need to wait 1 hour) – joseluisbz Dec 12 '19 at 02:57
  • I can see why it would block if you were using `Collections#synchronizedMap`, but not if you were just using `HashMap`. – Jacob G. Dec 12 '19 at 03:04
  • The result is the same, using `new HashMap<>();` or `Collections.synchronizedMap(new HashMap<>());` – joseluisbz Dec 12 '19 at 03:15
  • ```Each Runnable is started in this way listFuture().add(executorService().submit(this)); in its constructor.``` Sounds like "leaking" reference, try to avoid this. jfyi: https://stackoverflow.com/questions/9851813/java-leaking-this-in-constructor – Grigory Dec 17 '19 at 16:01
  • @Grigory Thank you for your recommendation, Already I Comment in its constructor `listFuture().add(executorService().submit(this));` by `SomeRunnable someRunnable = new SomeRunnable(blabla); listFuture().add(executorService().submit(someRunnable));` unfortunately The behaviour is same. – joseluisbz Dec 18 '19 at 22:58
  • You'll need to share *a lot* more code than you have here; even the video barely shows any code - you scroll around for a bit and it's quite difficult to see the actual code; most of it is console output. With code this complicated, it's going to take serious debugging to ID the problem. I'm talking thread dumps and JFR; not something one can just glance at and prescribe a solution. Good luck anyway – kolossus Dec 19 '19 at 23:30
  • Hi, I put all functional code reproducing the problem in github (taken from Netbeans). I was delayed by job and my personal project was delayed. I was reviewing but I can't find the problem. – joseluisbz Apr 07 '20 at 03:29
  • @kolossus Late, but do it! – joseluisbz Apr 07 '20 at 03:34

1 Answers1

3

First, the way you set thread names is wrong. You use this pattern:

public class Test
{
    public static class Task implements Runnable
    {
        public Task()
        {
            Thread.currentThread().setName("Task");
        }

        @Override
        public void run()
        {
            System.out.println("Task: "+Thread.currentThread().getName());
        }
    }

    public static void main(String[] args)
    {
        new Thread(new Task()).start();
        System.out.println("Main: "+Thread.currentThread().getName());
    }
}

which gives the (undesired) result:

Main: Task
Task: Thread-0

It's incorrect because, in the Task constructor, the thread has not started yet, so you're changing the name of the calling thread, not the one of the spawned thread. You should set the name in the run() method.

As a result, the thread names in your screenshot are wrong.

Now the real issue. In WorkerDSPIncrement, you have this line:

List<ChunkDTO> listDelayedChunkDTO = mapListDelayedChunkDTO.putIfAbsent(chunkDTO.getPitch(), new ArrayList<>());

The documentation for putIfAbsent() says:

If the specified key is not already associated with a value (or is mapped to null) associates it with the given value and returns null, else returns the current value.

Since the map is initially empty, the first time you call putIfAbsent(), it returns null and assigns it to listDelayedChunkDTO.

Then you create a ProcessorDSP object:

ProcessorDSP processorDSP = new ProcessorDSP(controlDSP, upNodeDSP, null,
   dHnCoefficients, chunkDTO, listDelayedChunkDTO, Arrays.asList(parent.getParentBlockingQueue()));

It means you pass null as the listDelayedChunkDTO parameter. So when this line executes in ProcessorDSP:

if (listDelayedChunkDTO.size() > 2) {

it throws a NullPointerException and the runnable stops.

Olivier
  • 13,283
  • 1
  • 8
  • 24
  • 1
    I see you also have an incorrect use of `putIfAbsent()` in [WorkerDSP](https://github.com/joseluisbz/ExecutorService_BlockingQueue_Problems/blob/master/src/joseluisbz/mock/support/WorkerDSP.java). And be aware that the second argument is always evaluated, even if the entry already exists! It means you will always create a `WorkerDSPPitchIncrement()` object, which is obviously not desired – Olivier Apr 07 '20 at 14:34
  • I understand that according to the implementation it should not return null. [putIfAbsent()](https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#putIfAbsent-K-V-) `V v = map.get(key); if (v == null) { v = map.put(key, value); } return v;` **I changed to** `V v = map.get(key); if (v == null) { map.put(key, value); v = map.get(key); } return v;` – joseluisbz Apr 07 '20 at 18:26
  • 1
    @joseluisbz [put()](https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#put-K-V-) returns "null if there was no mapping for key", so it's expected behaviour. – Olivier Apr 07 '20 at 18:29
  • What do you think what is better to use `List listDelayedChunkDTO = new ArrayList<>();` or `List listDelayedChunkDTO = Collections.synchronizedList(new ArrayList<>());` in *WorkerDSPDecrement*, *WorkerDSPFilter*, *WorkerDSPPitchIncrement* and `Map mapWorkerDSPPitchIncrement = new HashMap<>();`or `Map mapWorkerDSPPitchIncrement = Collections.synchronizedMap(new HashMap<>());` in *WorkerDSP* – joseluisbz Apr 07 '20 at 23:03
  • @joseluisbz Well it depends if the collections are accessed concurrently or not. If only a single thread accesses them, then there is no reason to make them synchronized. – Olivier Apr 08 '20 at 08:59