Cannot found entity after some and/remove operation

[endison1986]

Hi @enricostara , this is my test code

var d = Dominion.create();
var entity = d.createEntity(new A());
var s = d.createScheduler();
s.schedule(() -> {
    d.findEntitiesWith(A.class, C.class).stream().forEach(rs -> {
        try {
            System.out.println("AAAA");
            if (!rs.entity().has(B.class)) {
                rs.entity().add(new B());
            }
        } finally {
            rs.entity().removeType(C.class);
        }
    });
});
s.schedule(() -> {
    d.findEntitiesWith(A.class, B.class).stream().forEach(rs -> {
        System.out.println("BBBBBB");
    });
});
/// always handle the A.class
s.schedule(() -> {
    System.out.println("Before CCCC");
    d.findEntitiesWith(A.class).stream().forEach(rs -> {
        System.out.println("CCCCCCCCCCCC");
    });
});
s.tickAtFixedRate(1);
TimeUnit.SECONDS.sleep(2);
entity.add(new C());

I have three component （A,B,C）and three system, always have a system to handle A.class. In the initial situation, the system operation is normal，but when I add C.class and add B.class then remove C.class, the system is not working

My version is 0.9.0-SNAPSHOT, you can run my test code and see the output.

[endison1986]

I debugged the source code and found the problem.

/// dev.dominion.ecs.engine.ResultSet.IteratorWrapper
private static final class IteratorWrapper<T> implements Iterator<T> {
    private final ResultSet<T> owner;
    private final Iterator<CompositionRepository.Node> nodesIterator;
    private Iterator<T> wrapped;

    public IteratorWrapper(ResultSet<T> owner, Iterator<CompositionRepository.Node> nodesIterator) {
        this.owner = owner;
        this.nodesIterator = nodesIterator;
        this.wrapped = this.nodesIterator.hasNext() ?
                owner.compositionIterator(this.nodesIterator.next().getComposition()) :
                new Iterator<>() {
                    @Override
                    public boolean hasNext() {
                        return false;
                    }

                    @Override
                    public T next() {
                        return null;
                    }
                };
    }

    @Override
    public boolean hasNext() {
        return wrapped.hasNext()
                || (nodesIterator.hasNext() && (wrapped = owner.compositionIterator(nodesIterator.next().getComposition())).hasNext());
    }

    @Override
    public T next() {
        return wrapped.next();
    }
}

The problem in hasNext() method, the nodesIterator has 4 node, and the entity in the last node Node={types=[A,B]}

{IndexKey@1539} "|1:null|" -> {CompositionRepository$Node@1540} "Node={types=[A]}"
{IndexKey@1541} "|30817:[1, 2, 3]|" -> {CompositionRepository$Node@1542} "Node={types=[A,B,C]}"
{IndexKey@1557} "|994:[1, 2]|" -> {CompositionRepository$Node@1558} "Node={types=[A,C]}"
{IndexKey@1559} "|995:[1, 3]|" -> {CompositionRepository$Node@1560} "Node={types=[A,B]}"

But the hasNext() method only view the first two nodes and return false

[endison1986]

I try to fix this issue, and test my code, it seems to work.

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

    while (nodesIterator.hasNext()) {
        if ((wrapped = owner.compositionIterator(nodesIterator.next().getComposition())).hasNext()) {
            return true;
        }
    }

    return false;
}

[enricostara]

Hi @endison1986 !
Thanks so much for testing and debugging! Let me check all your findings, I'll give you an update very soon.

[enricostara]

Hi @endison1986 ,

I really appreciate your effort and it would be great if you started as a project contributor.

First step, maybe take it to the next level by trying to write a specific unit test to highlight the problem you discovered.

Even if you're working on multi-threaded examples, you might want to try distilling the issue to see if it's really about concurrency.
This is even because, as a general rule, unit tests should be as single-threaded as possible - although there are some situations where you can't help it.

For example, this is the unit test I'm about to add into the suite starting from your multi-threaded example (and without any patch the last assert fails as expected):

    @Test
    void findEntitiesAddingAndRemovingComponents() {
        EntityRepository entityRepository = (EntityRepository) new EntityRepository.Factory().create("test");

        var entity = entityRepository.createEntity(new C1(0));
        var iterator = entityRepository.findEntitiesWith(C1.class).iterator();
        Assertions.assertTrue(iterator.hasNext());

        entity.add(new C2(0));
        iterator = entityRepository.findEntitiesWith(C1.class).iterator();
        Assertions.assertTrue(iterator.hasNext());

        entity.add(new C3(0));
        iterator = entityRepository.findEntitiesWith(C1.class).iterator();
        Assertions.assertTrue(iterator.hasNext());

        entity.removeType(C3.class);
        iterator = entityRepository.findEntitiesWith(C1.class).iterator();
        Assertions.assertTrue(iterator.hasNext());
    }

I'm not using the scheduler as the problem you discovered is not with concurrency but is a plain bug.
Now, if you apply your own patch, it works as expected.

So you could just create a pull request from your fork and submit your commit with a unit test like this and the patch you created.
After my review your code will (hopefully) be merged and you will be in the list of committers 👍

[endison1986]

Hi, @enricostara
Thank you for your trust and help, I feel very honored, this is the first time I create a PR on github, so I'm not sure if the creation was successful. I forked the project, created unit tests and modified the code, then pushed it to my private project, and then created a PR. I don’t know if my steps are correct. If there is something wrong, please forgive me.

[enricostara]

Fixed by #139