Java issues with notify() / wait()

Question

I have a generator inside an Iterator instance, which generates and returns one item every time next() is called on the iterator. It does seem to kind of work, but I am getting null values returned.

Class code:

package spiral;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Iterator;
import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;

/**
 *
 * @author student
 */
public class Spiral7DGenerator implements Iterator<List<Integer>> {
    private boolean releaseNext = false;
    private List<Integer> releaseList;

    public Spiral7DGenerator() {
        new Thread(new Runnable() {
            @Override
            public void run() {
                for (int t = 0; true; t++) {
                    loops(t);
                }
            }
        }).start();
    }

    @Override
    public boolean hasNext() {
        return true;
    }

    @Override
    public List<Integer> next() {
        synchronized(this) {
            releaseNext = true;
            notify();
            return releaseList;
        }
    }

    @Override
    public void remove() {
        throw new UnsupportedOperationException("Not supported yet.");
    }

    private void loops(int t) {
        for (int d1 = 0; d1 <= t; d1++) {
            for (int d2 = 0; d2 <= (t - d1); d2++) {
                for (int d3 = 0; d3 <= (t - d1 - d2); d3++) {
                    for (int d4 = 0; d4 <= (t - d1 - d2 - d3); d4++) {
                        for (int d5 = 0; d5 <= (t - d1 - d2 - d3 - d4); d5++) {
                            for (int d6 = 0; d6 <= (t - d1 - d2 - d3 - d4 - d5); d6++) {
                                int d7 = (t - d1 - d2 - d3 - d4 - d5 - d6);
                                generate(0, d1, d2, d3, d4, d5, d6, d7);
                            }
                        }
                    }
                }
            }
        }
    }

    private void generate(int pos, Integer... array) {
        if (pos == array.length) {
            List<Integer> list = new ArrayList<>();
            list.addAll(Arrays.asList(array));
            synchronized(this) {
                while (!releaseNext) {
                    try {
                        wait();
                    } catch (InterruptedException ex) {
                        Logger.getLogger(Spiral7DGenerator.class.getName()).log(Level.SEVERE, null, ex);
                    }
                }
            }
            releaseNext = false;
            releaseList = list;
            return;
        }
        generate(pos + 1, array);
        array[pos] = -array[pos];
        if (array[pos] != 0) {
            generate(pos + 1, array);
        }
    }
}

Testing code:

package spiral;

import org.junit.Test;
import static org.junit.Assert.*;

/**
 *
 * @author Beheerder
 */
public class Spiral7DGeneratorTest {

    public Spiral7DGeneratorTest() {
    }

    @Test
    public void testHasNext() {
    }

    @Test
    public void testNext() {
        System.out.println("test");
        Spiral7DGenerator s7dg = new Spiral7DGenerator();
        System.out.println("s7dg.next() = " + s7dg.next());
        System.out.println("s7dg.next() = " + s7dg.next());
        System.out.println("s7dg.next() = " + s7dg.next());
        System.out.println("s7dg.next() = " + s7dg.next());
    }

    @Test
    public void testRemove() {
    }

}

Test output:

test
s7dg.next() = null
s7dg.next() = null
s7dg.next() = null
s7dg.next() = null

I'm afraid it is just a simple thing, but I am overlooking it completely.

Do you really need to write such a complex code. It is quite unreadable and hard to understand for me atleaset. I am running out of memory while reading those nested for loops. — Juned Ahsan
– Juned Ahsan, Commented Aug 12, 2013 at 7:12
@JunedAhsan While the question itself is not about that code, I can explain why it is such complex. That is because making spirals in general is complex, of course I would love to make it generic instead of this, but that is currently simply too hard for me. — skiwi
– skiwi, Commented Aug 12, 2013 at 7:13
Your loop in Spiral7DGenerator() const is never ending and is starting new thread on each iteration..Its a mistake (as memory is limited) — harrybvp
– harrybvp, Commented Aug 12, 2013 at 7:16
Using multiple threads is rarely a way to make your code simpler. I still think this is much simpler and more likely to work stackoverflow.com/questions/18074982/… — Peter Lawrey
– Peter Lawrey, Commented Aug 12, 2013 at 8:02

Adrian Pronk · Accepted Answer · 2013-08-12 07:28:21Z

5

I haven't fully understood your code but two points I can make:

releaseList must be declared volatile (at least) as it is being assigned outside a synchronized block.
your next method does not wait for the notified thread to wake up and produce a result. This is a race condition. You need next to wait on an extra mutex (or something) to allow the generator to notify that releaseList has been set. (Actually, when you do this, you won't need to make releaseList volatile any more.)

edited Aug 12, 2013 at 7:28

answered Aug 12, 2013 at 7:19

Adrian Pronk

14k7 gold badges38 silver badges61 bronze badges

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

1 Comment

Tom Anderson Over a year ago

I suspect it's the fact that next() does not wait for the worker thread at all that's leading to all the nulls. You're absolutely right about releaseList (and releaseNext) not being safely updated, too.

Tom Anderson · Accepted Answer · 2013-08-12 08:10:33Z

2

The idea here is that the worker thread feeds a stream of results to the main thread, correct?

I suggest that you do not attempt to construct the concurrency control logic by hand, but instead use a SynchronousQueue. The code would look like:

public class Spiral7DGenerator implements Iterator<List<Integer>> {
    private BlockingQueue<List<Integer>> spirals = new SynchronousQueue<List<Integer>>();

    @Override
    public List<Integer> next() {
        return spirals.take();
    }

    private void generate(int pos, Integer... array) {
        if (pos == array.length) {
            List<Integer> list = new ArrayList<>();
            list.addAll(Arrays.asList(array));
            spirals.put(list);
            return;
        }
    // etc
    }
}

answered Aug 12, 2013 at 8:10

Tom Anderson

47.4k17 gold badges96 silver badges138 bronze badges

6 Comments

skiwi Over a year ago

Thanks! This is exactly what I need. It is such a shame though that Java has no Python's yield equivalent.

Tom Anderson Over a year ago

It certainly is. Scala, of course, has added continuations which let you build things like generators. I think that's unlikely to ever come to Java, though.

skiwi Over a year ago

Actually, this might not be 100% what I want. I want generate() to only do something when spirals.isEmpty(). Can the BlockingQueue be (easily) modified such that it blocks when spirals.size() > threshold? As I would like it to block/wait whenever spirals.size() > 0. Then wait until next() has been called, and then generate the next element again, etc.

Tom Anderson Over a year ago

As it is, spirals.put will block until the main thread comes along and calls spirals.take, so there will only ever be one value passing through the queue at a time. If you want to have some larger but still fixed limit on the size of the queue, replace the SynchronousQueue with an ArrayBlockingQueue of whatever capacity you need.

skiwi Over a year ago

Ah ok so SynchronousQueue is indeed exactly what I need, I should have read the documentation before asking.

|

Collectives™ on Stack Overflow

Java issues with notify() / wait()

2 Answers 2

1 Comment

6 Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

1 Comment

6 Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related