Async method not processed - wrong generated #11

Basic information

TDE version: R14.1
Distribution: Debian Stretch
Hardware: any

Description

when method is annotated as Async, the generated code does not work because handling is wrong

Steps to reproduce

use this in the xml

        <interface name="org.example.Service">
                <method name="ListSorter">
                        <arg name="input" type="as" direction="in" />
                        <arg name="output" type="as" direction="out" />
                </method>
                <method name="Test">
                        <annotation name="org.freedesktop.DBus.GLib.Async"/>
                        <arg name="input" type="s" direction="in" />
                        <arg name="output" type="s" direction="out" />
                </method>
        </interface>
</node>

generate the code
in serviceinterface.cpp

bool ServiceInterface::handleMethodCall(const TQT_DBusMessage& message)
...
...
    if (message.member() == "Test")
    {
        callTestAsync(message);

        return true;
    }

    if (message.member() == "Test")
    {
        TQT_DBusMessage reply = callTest(message);
        handleMethodReply(reply);

        return true;
    }
...
...

It should be

bool ServiceInterface::handleMethodCall(const TQT_DBusMessage& message)
...
...
    if (message.member() == "TestAsync")
    {
        callTestAsync(message);

        return true;
    }

    if (message.member() == "Test")
    {
        TQT_DBusMessage reply = callTest(message);
        handleMethodReply(reply);

        return true;
    }
...
...

see commit 030ba0231b in pull #8

After building the code and testing I found out that now if we call dbus Introspect, it will return the xml with the async method generated twice

see commit a0cf1c4793 in pull #8, however I think the approach is wrong. it should generate the method with annotation. However in this case it is hard to tell which method we want to use as visible in this issue.

Perhaps we should prevent generation of non async method with same name, because this method is marked as async - this will require quite of a code change I think.

There is interesting discussion around this.

So in general to add "Async" is wrong because "TestAsync" is not in the introspection. To me it looks like in case a method is annotated as async - the blocking method should not be handled at all and vice versa.

This will require more brain power.

I opened a new pull request #12 - I would remove those commits from here, but I wait for you to comment.

I do not understand why you introduce a new property "method.haveAsync". IMO "method.async" is sufficient in this case. As you and Michele are the main drivers, I would suggest he has to look into it, I am happy when it works like it should. I will test in the evening (hopefully), what code is exactly generated and how useful it is.

My intention is simple – not to make the code unnecessarily complex.

At the moment of introspection generation, you have only the current method information available. When you see that (*it).async is false => introspection will be generated, but you don't know if there is the same method in the list as async. You would have to search the list to see if you can find that async method. In my proposal, the information about the existence of the async method is kept at the beginning – the sync method has the set flag (*it).haveAsync to true. Information about the existence of the async method is therefore known immediately – simple.

Of course, you can ignore my commit if you have a simpler solution 😸

I will take a look sooner or later and let you know my view as well. I prefer to go one step at a time in the correct sequence. Yesterday I started looking at #7 and immediately ran into the problem described in #17. So IMO next step is to fix #17, then we can go back to #7 and #11.

The solution for this issue seems to be simple and independent of the more extensive changes that are in other issues. Therefore, I thought it would be possible to complete and close it. This will help to make fewer pending changes. Just my two cents.

OK, thanks Slavek, I understand now. I was thinking checking if size is > 1 is enough. Let us see what Michele will do. Thank you!

Fixed by PR #12.