Is it necessary to add synchronized to my code?

Question

the following code sometimes generates NullPointerException Exception at ret.forEach(v -> System.out.println(v)); line. I think I have to use synchronized block or Lock Interface to avoid this error. Is it correct? Please tell me some advice.

List<Integer> ret = new ArrayList<>();
IntStream.range(0, 10).parallel().forEach(i -> {            
        if (i % 2 == 0) {
            try {
                System.out.println("stop" + i);
                Thread.sleep(10000);
            } catch (InterruptedException e) {
                // TODO Auto-generated catch block
                e.printStackTrace();
            }
        }
        ret.add(i);
    });         
    ret.forEach(v -> System.out.println(v));

You define `ret` after you use it in the `IntStream`. Is that a typo? — Joachim Sauer, Oct 09 '20 at 09:10

score 2 · Answer 1 · answered Oct 09 '20 at 09:12

2

Basically yes: you modify an ArrayList from multiple threads. ArrayList is not thread-safe, so doing that can cause any number of issues (there is no single problem/exception that will always happen).

Null-values being present when they shouldn't be is one of the possible outcomes of using non-thread collections.

The best way to produce a list from a stream is therefore not to use forEach and explicitly add something to a list, but to use map and collect.

answered Oct 09 '20 at 09:12

Joachim Sauer

302,674
57
556
614

Thanks! You mean that I should replace ArrayList with map or collection in my code, don't you? – RRR Oct 09 '20 at 23:41
No, I am talking about using different methods on the `Stream`: instead of building the functionality of `Stream.collect` manually using `forEach`, you should use [`Stream.collect`](https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/stream/Stream.html#collect(java.util.stream.Collector)). – Joachim Sauer Oct 10 '20 at 06:58

score 1 · Accepted Answer · answered Oct 09 '20 at 09:17

1

There are some things here to comment:

You can't use ret before its declaration.
You are adding elements to an ArrayList in parallel. ArrayList is not threadsafe so you shouldn't use it there. Look at Choosing the best concurrency list in Java.

Finally, if you are using streams, its better to use a map and a collect. Something similar to this:

IntStream.range(0, 10)
  .parallel()
  .map(i -> do_watever(i))
  .collect(Collectors.toList());;

answered Oct 09 '20 at 09:17

Daniel Argüelles

2,229
1
33
56

Thanks! I'll see this URL. It is good to use your 3. from the viewpoint of thread-safe – RRR Oct 09 '20 at 23:40

Is it necessary to add synchronized to my code?

2 Answers2