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

Signal connection does not work as expected with generics #69

Closed
kbakaras opened this issue Aug 16, 2022 · 13 comments
Closed

Signal connection does not work as expected with generics #69

kbakaras opened this issue Aug 16, 2022 · 13 comments

Comments

@kbakaras
Copy link

Describe the bug
Connecting method call to signal has unexpected and erroneous behavior with generics. For example, connection with method reference does not work, but connection with equivalent lambda does. Problem arises when you emit signal with such connection. And wise versa, in some cases exception appears at once I try to connect lambda.

I suspect method private synchronized <Slot extends Serializable> QMetaObject.Connection addConnectionToSlotObject(SlotConnectionFactory<Slot> factory, Slot slotObject, Qt.ConnectionType[] connectionType) in QtJambiSignals class.

To Reproduce
I've simplified my cases to unit-test class. It's runnable with junit 5.8.2 (jupiter). Here it is entirely:

package io.qt.core;

import io.qt.widgets.QApplication;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;

class SignalTest {

    static class ReceiverObject<E> extends QObject {

        public final Signal1<E> elementModified = new Signal1<>();

        public ReceiverObject() {
            elementModified.connect(this::elementModified);
        }

        public void elementModified(E element) {
            System.out.println(element);
        }

    }

    static class Registrar<E> extends QObject {
        final Signal1<E> modified = new Signal1<>();
    }

    private final Registrar<String> registrar = new Registrar<>();


    @BeforeAll
    static void init() {
        QApplication.initialize("TEST", new String[0]);
    }

    @AfterAll
    static void done() {
        QApplication.shutdown();
    }


    // Gives 'io.qt.QMisfittingSignatureException: Incompatible sender/receiver arguments modified(QVariant) -> private void io.qt.core.SignalTest$1.lambda$new$896dfc3$1(java.lang.String) throws java.lang.Throwable'
    @Test
    void testLambda() {

        ReceiverObject<String> receiver = new ReceiverObject<>() {
            {
                registrar.modified.connect(element -> elementModified(element));
            }
        };
        receiver.elementModified("testLambda");

        registrar.modified.emit("...passed");
    }

    // Works as expected
    @Test
    void testOuterLambda() {

        ReceiverObject<String> receiver = new ReceiverObject<>();
        receiver.elementModified("testOuterLambda");
        registrar.modified.connect(element -> receiver.elementModified(element));

        registrar.modified.emit("...passed");
    }

    // Gives 'Process finished with exit code 139 (interrupted by signal 11: SIGSEGV)'
    @Test
    void testOuterMethodReference() {

        ReceiverObject<String> receiver = new ReceiverObject<>();
        receiver.elementModified("testOuterLambda");
        registrar.modified.connect(receiver::elementModified);

        registrar.modified.emit("...passed");
    }

    // Gives 'Process finished with exit code 139 (interrupted by signal 11: SIGSEGV)'
    @Test
    void testMethodReference() {

        ReceiverObject<String> receiver = new ReceiverObject<>() {
            {
                registrar.modified.connect(this::elementModified);
            }
        };
        receiver.elementModified("testMethodReference");

        registrar.modified.emit("...passed");
    }

    // Gives 'Process finished with exit code 139 (interrupted by signal 11: SIGSEGV)'
    @Test
    void testSignalToSignal() {

        ReceiverObject<String> receiver = new ReceiverObject<>();
        receiver.elementModified("testSignalToSignal");
        registrar.modified.connect(receiver.elementModified);

        registrar.modified.emit("...passed");
    }

}

Expected behavior
I expect that in all aforementioned cases connection is functional. And there should not be any difference between using method reference and lambda as slot. And the most uncomfortable was SIGSEGV results.

System:

  • OS: Arch linux
  • Java version 11
  • QtJambi version 6.3.2
  • Qt version 6.3.1

Additional context

  1. I suppose SIGSEGV may be specific to linux, and on windows or on mac it may sound different. But I tried it on linux only.
  2. If I change Registrar to not generic (with String-parameter signal) most of tests will pass (except the testSignalToSignal).
@omix
Copy link
Contributor

omix commented Aug 16, 2022

Thanks for the report!
Using generics as signal/slot parameters have never been checked up to now. Java does not know the actual instantiations at runtime. However, it should at least map to Object and work without crashing.

@omix
Copy link
Contributor

omix commented Aug 16, 2022

Did you use QtJambi native binaries from Maven or did you compile it yourself?

@omix
Copy link
Contributor

omix commented Aug 16, 2022

I checked your code. The reason for the crash is not the use of generics. It is the line elementModified.connect(this::elementModified);. Here, a recursive call is initialized. Both, the signal field elementModified and the method elementModified represent the same internal QMetaMethod elementModified. Thus, you connect the signal elementModified to itself causing a stackoverflow in native code which leads to crash.

@kbakaras
Copy link
Author

Did you use QtJambi native binaries from Maven or did you compile it yourself?

I use io.qtjambi:qtjambi-native-linux-x64:6.3.2 from maven.

@kbakaras
Copy link
Author

However, it should at least map to Object and work without crashing.

Yes, it would be correct. All other type-checking is compiler's job.

@omix
Copy link
Contributor

omix commented Aug 16, 2022

Try to rename method elementModified in ReceiverObject. The name must be different from the signal name. Then try again.

@kbakaras
Copy link
Author

kbakaras commented Aug 16, 2022

Try to rename method elementModified in ReceiverObject. The name must be different from the signal name. Then try again.

Yes, I was renaming signal in ReceiverObject just now. You are right. Everything starts working after that. But it's not obvious!

I will consider PR-ing some docs about signal usage subtleties in qtjambi, if you don't mind.

Thanks for your response!

@kbakaras
Copy link
Author

The only unexplained yet is test testLambda.

@omix
Copy link
Contributor

omix commented Aug 16, 2022

I'm currently checking it.

@omix
Copy link
Contributor

omix commented Aug 16, 2022

Unfortunately, it is not possible to calculate recursive signal connections in Java. Well, even native Qt doen't check it. I Also don't want to add a test if you are connecting a signal to itself. That costs additional runtime for something programmers normally wouldn't do. Here, we have the misinterpretation caused by Java allowing methods and fields to have the same name.

What I have done now is to treat a method handle as lambda expression if the Java method points to a signal.
Thus, receiver::elementModified is the same as element -> elementModified(element). Normally, QtJambi tries to resolve Java method handles to QMetaMethods.

@omix
Copy link
Contributor

omix commented Aug 16, 2022

The only unexplained yet is test testLambda.

QtJambi simply does not support variable generic types like <E>. But that's easy to implement. I will incorporate it in the next release.

@kbakaras
Copy link
Author

Great! Thank you!

omix added a commit that referenced this issue Aug 19, 2022
Bugfix Issue #60
Bugfix Issue #62
Bugfix Issue #63
Bugfix Issue #64
Bugfix Issue #66
Bugfix Issue #69
@omix
Copy link
Contributor

omix commented Aug 22, 2022

Resolved in QtJambi 6.3.3 / 6.2.6 / 5.15.8

@omix omix closed this as completed Aug 22, 2022
omix added a commit that referenced this issue Jul 25, 2023
Bugfix Issue #60
Bugfix Issue #62
Bugfix Issue #63
Bugfix Issue #64
Bugfix Issue #66
Bugfix Issue #69
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

2 participants