Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Semaphore causes AssertJ to hang indefinitely. #1966

Closed
rdehuyss opened this issue Aug 14, 2020 · 5 comments
Closed

Semaphore causes AssertJ to hang indefinitely. #1966

rdehuyss opened this issue Aug 14, 2020 · 5 comments
Milestone

Comments

@rdehuyss
Copy link

Summary

I'm using Objects which contains Semaphore's. When using recursive comparison, AssertJ hangs indefinetly.

I cannot use ReentrantLock because of non-ownership-release semantics.

Example

package org.jobrunr.utils;

import org.junit.jupiter.api.Test;

import java.util.Arrays;
import java.util.List;
import java.util.concurrent.Semaphore;

import static org.assertj.core.api.Assertions.assertThat;

public class SemaphoresFail {

    @Test
    public void compareListWithObjectsContainingSemaphores() {
        final SomeObjectWithALock someObjectWithALock1 = new SomeObjectWithALock();
        final SomeObjectWithALock someObjectWithALock2 = new SomeObjectWithALock();

        // hangs here
        assertThat(someObjectWithALock1).usingRecursiveComparison().isEqualTo(someObjectWithALock2);

        List list1 = Arrays.asList(someObjectWithALock1);
        List list2 = Arrays.asList(someObjectWithALock2);

        // hangs here
        assertThat(list1)
                .usingRecursiveFieldByFieldElementComparator()
                .containsAll(list2);

    }


    private class SomeObjectWithALock {
        private transient final SemaphoresFail.Lock lock;

        public SomeObjectWithALock() {
            this.lock = new SemaphoresFail.Lock();
        }


        public SemaphoresFail.Lock lock() {
            return lock.lock();
        }
    }


    private static class Lock implements AutoCloseable {

        private final Semaphore semaphore;

        public Lock() {
            this.semaphore = new Semaphore(1);
        }

        public Lock lock() {
            semaphore.acquireUninterruptibly();
            return this;
        }

        public boolean isLocked() {
            return this.semaphore.availablePermits() < 1;
        }

        public void close() {
            unlock();
        }

        public void unlock() {
            semaphore.release();
        }
    }
}
@rdehuyss
Copy link
Author

Further investigation shows that AssertJ only hangs because of the name of the following method:

// makes AssertJ to hang
public SemaphoresFail.Lock lock() {
        return lock.lock();
}

If the method is renamed, it does not hang.

// no problem here
public SemaphoresFail.Lock lockObject() {
        return lock.lock();
}

rdehuyss pushed a commit to jobrunr/jobrunr that referenced this issue Aug 14, 2020
@joel-costigliola joel-costigliola added this to the 3.18.0 milestone Aug 28, 2020
kriegfrj added a commit to kriegfrj/assertj-core that referenced this issue Sep 20, 2020
@joel-costigliola
Copy link
Member

@rdehuyss sorry for the late answer, I have finally took the time to look into it.

The issue is caused by AssertJ executing the lock() method multiple times to get the lock field value thinking it is a getter for the lock field.

AssertJ looks for getter methods in two different ways:

  • the classic bean getter, ex: getLock()
  • the bare version, ex: lock()

The getter method is invoked multiple times to evaluate whether the field should be ignored and to get its value for comparison, it is assumed the getter method has no side effect (assumption obviously wrong in your case).

One way to fix your issue without renaming the field (as you did) is to disable bare name getter usage with:

Assertions.setExtractBareNamePropertyMethods(false);

The default is to look for bare name getters which we changed a few months ago but I'm not completely sure it was a good idea (I'll discuss with the team to see if we agree reverting to disabling bare name getter by default).

@joel-costigliola
Copy link
Member

We are reverting to not look for bare name getters but for users who wants this behaviour simply call:

Assertions.setExtractBareNamePropertyMethods(true);

@filiphr
Copy link
Contributor

filiphr commented Oct 12, 2020

Just out of curiosity does this also mean that Java 14 records are not working? Asking since with some reflection it is quite easy to get the accessor methods for a java.lang.Record.

@joel-costigliola
Copy link
Member

joel-costigliola commented Oct 13, 2020

@filiphr we haven't tried yet, I have created an issue to check that #2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants