3

I have several computing threads that create result values (Objects). After each thread is finished a 'add' method from a result collector is called. This result collector is singleton, so there is only one representation.

Inside the result collector is a list which holds result objects:

List<TestResult> results = Collections.synchronizedList(new ArrayList<>());

The add method adds the result of each thread to the list:

  public void addResult(TestResult result){
        System.out.println("added " + result.getpValue());
        this.results.add(result);
    }

It is called within the thread, after the computing stuff is done.

The big problem is: After all threads are finished the list of results is empty. As you can see in the addResult method I added a print statement for the pValue. The p value of all results is printed out.

So it looks like the threads work on different lists. Despite the fact that the collector class is singleton.

It was asked for the complete code of the result collector (Javadoc removed to trim size)

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;


public class ResultCollector {

    private static ResultCollector resultCollector;
    private final List<TestResult> results;

    public static ResultCollector getInstance(){
        if(resultCollector == null){
            resultCollector = new ResultCollector();
        }

        return resultCollector;
    }

    private ResultCollector() {
        results = Collections.synchronizedList(new ArrayList<>());
    }

    public void addResult(TestResult result){
        System.out.println("added " + result.getpValue());
        this.results.add(result);
    }
}

I updated the add method to print out the hash of the current class to make sure every thread has the same with: System.out.println("added to" + System.identityHashCode(this) + " count: " +results.size());

The output hash code is the same for all threads and the size increases to the expected value. Also the hash code is the same when I call my toString method or getter for the list outside the multithread environment.

Calling of the threads:

public IntersectMultithread(...) {

    Set<String> tracks = intervals.keySet();

    for(String track: tracks){

        IntersectWrapper wrapper = new IntersectWrapper(...);
        wrappers.add(wrapper);
        exe.execute(wrapper);
    }

    exe.shutdown();
}
7
  • Can you provide the Collector (Singleton) class source? Commented Jan 26, 2016 at 15:29
  • Pls provide your code... Commented Jan 26, 2016 at 15:32
  • I don't see anything wrong here as of yet... unless I'm missing something. We might need more of your code. Is your thread-execution code compact enough to post? Commented Jan 26, 2016 at 15:40
  • You mean the part where I call/execute the threads? Or one level deeper where the computation is called and the results are added to the collector? Commented Jan 26, 2016 at 15:46
  • 1
    A side note: That singleton itself is not thread-safe. Do a private static final ResultCollector resultCollector = new ResultCollector();, and omit the ==null check in the getInstance method. Commented Jan 26, 2016 at 16:17

2 Answers 2

3

A Synchronized list is just a wrapper over a list. You should actually be using Concurrent Collections for this purpose in modern Java; they implement smarter and more efficient locking and provide better performance.

Caveat: the only synchronized list is one that copies on write. So, if that's an issue (i.e. you're adding more than iterating), then your way is fine).*

The error in your code is almost certainly in your singleton class which you haven't shown. Either your class is not truly a singleton (did you use an enum? that's a good way to guarantee it), or your list creation is more confusing than let on.

If you post more code, I can update the answer with more info :).


EDIT: I think your problem is here based on your updated code:

exe.shutdown();

You need to wait for the executor to complete with awaitTermination() with a good timeout relevant to the work you are doing.

Your threads just start and die off instantly right now :)

For example:

taskExecutor.awaitTermination(Long.MAX_VALUE, TimeUnit.NANOSECONDS);

From here: https://stackoverflow.com/a/1250655/857994

Sign up to request clarification or add additional context in comments.

7 Comments

@GregHilston I don't sit around waiting for comments to show up. I just provide whatever useful information I can and update later when I have time (I'm working and trying to be a helpful person, not sitting on SO for points). This site's for giving people some help and my answer gave much more help then your one-liner.
@GregHilston Depends on whether your "placeholder" crosses the line into a potentially useful, though broad, answer to a broad question. This one does, though barely. Plain clarification is a comment.
@JohnHumphreys-w00te Not sure if you were upset with my comment, but I was asking as you are obviously a much more experience user here and was trying to learn on how to appropriately ask for more information. Just wanted to clarify my intentions. Thanks for the information. Chrylis, thank you as well
@GregHilston - Oh sorry Greg, I apologize then. I used to hate it when people were snippy on SO; so many people are (I think it's an engineer thing). Honestly, as long as you're trying to help out with whatever you post, its fine on here =). My apologies for the confusion; definitely misinterpreted your comment. Anyway, comments are for useful stuff; so I'll stop after this one.
@JohnHumphreys-w00te Not a problem at all! Its incredibly difficult to read the intended "tone" of voice that is being portrayed over a pure text communication, which is why I wanted to clarify. Thank you for teaching me!
|
0

Addition to the correct answer above

Yes, the exe.shutdown(); is the problem, but the threads do not die instantly, instead they seem to run through. This is why the 'add' method printed everything correct if extended with a print.

The issue was that my output was done before the threads could finish their computation. So there were no values at that time, shortly after the threads finish and the print works.

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.