#11 Async method not processed - wrong generated

Closed
opened 3 months ago by deloptes · 10 comments

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

  1. 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>

  1. generate the code
  2. 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;
    }
...
...
<!-- This is a comment. Please fill in the required fields below. The comments provide instructions on how to do so. Note: You do not need to remove comments. --> ## Basic information - TDE version: R14.1 - Distribution: Debian Stretch - Hardware: any <!-- Use SL/* labels to set the severity level. Please do not set a milestone. --> ## Description when method is annotated as Async, the generated code does not work because handling is wrong ## Steps to reproduce 1. use this in the xml ```<node name="/org/example/service"> <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> ``` 2. generate the code 3. 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; } ... ... ```
deloptes commented 3 months ago
Poster

see commit 030ba0231b in pull #8

see commit https://mirror.git.trinitydesktop.org/gitea/TDE/dbus-1-tqt/commit/030ba0231b46559a1a026a656693aeeaf0d44275 in pull #8
deloptes commented 3 months ago
Poster

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.

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.
deloptes commented 3 months ago
Poster

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.

There is interesting [discussion around this](https://lists.freedesktop.org/archives/dbus/2006-September/005639.html). 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.
deloptes commented 3 months ago
Poster

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

I opened a new pull request #12 - I would remove those commits from here, but I wait for you to comment.
SlavekB referenced this issue from a commit 3 months ago
SlavekB referenced this issue from a commit 3 months ago
deloptes commented 3 months ago
Poster

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.

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.
SlavekB commented 3 months ago
Owner

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 :smile_cat:

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 :smile_cat:
MicheleC commented 3 months ago
Owner

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.

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.
SlavekB commented 3 months ago
Owner

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.

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.
deloptes commented 3 months ago
Poster

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!

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!
MicheleC commented 1 month ago
Owner

Fixed by PR #12.

Fixed by PR #12.
MicheleC added this to the R14.1.0 release milestone 1 month ago
Sign in to join this conversation.
No Milestone
No Assignees
3 Participants
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
Cancel
Save
There is no content yet.