Bug #7582
closedRegression: Wt::Signal emit() provides incorrect data in some cases
0%
Description
With the github version of Wt 4.3.0-26-gd18ca1b9, some Wt::Signal emit calls do not provide the expected data to their connected slots.
The issue can be observed with the hangman.wt example. After logging in (on the deployment path), the URL will end with "/play" but the page will not display the expected image or the "Ready to Play?" prompt. If you refresh the page and login again, the normal "Ready to Play?" page will appear and clicking on "New game" will start a game as expected. Clicking on "Highscores" briefly updates the URL to end with "/highscores", but then returns to "/play" and the displayed page remains unchanged.
I bisected the hangman issue to commit 4.3.0-19-g84043ff7, which added perfect forwarding for signals.
The internalPathChanged() Signal in hangman is connected to instances of both the AuthWidget and HangmanGame. When the internalPathChanged signal is emitted, the AuthWidget receives the expected std::string value and the HangmanGame receives an empty std::string.
My knowledge of templates is limited, but I believe the issue is due to how std::forward is being called in Wt/WSignal.h and Wt/Signals/signals.hpp. I suspect that forwarding references may be required for the std::forward calls to work as expected in all cases.
To test this theory, I applied the attached patch (very rough) which adds forwarding references and it seems to help with the hangman example.
The patch is only intended for illustration. It doesn't enforce that the arguments to emit() match the associated Signal. Also, it doesn't address the other std::forward calls in Wt/Signals/signals.hpp (ConnectHelper). Finally, I did not verify that perfect forwarding of signal arguments works correctly.
Files
Updated by Roel Standaert almost 4 years ago
Oof, good catch. I guess I should've scrutinized that pull request a bit more closely. It's unfortunate that it leads to a compile error when passing a null pointer as a 0 instead of nullptr. Of course nullptr is nicer, but it would mean that some of our users' code may break.
Updated by Roel Standaert almost 4 years ago
I elaborated on your patch a bit more, but then I wrote this test:
Wt::Signal<std::string> signal;
signal.connect([](const std::string &s) {
BOOST_REQUIRE(s == "Hello");
});
signal.connect([](const std::string &s) {
BOOST_REQUIRE(s == "Hello");
});
signal.emit(std::string("Hello"));
That fails because the prvalue from std::string("Hello") is moved into the first slot. The second slot gets an empty string. I think that's really bad.
This is fine, and in the old signals implementation (in Wt 4.3 and earlier) its performance is probably also better, because it prevents extra copies:
Wt::Signal<const std::string&> signal;
signal.connect([](const std::string &s) {
BOOST_REQUIRE(s == "Hello");
});
signal.connect([](const std::string &s) {
BOOST_REQUIRE(s == "Hello");
});
signal.emit(std::string("Hello"));
However, I don't think it's a good idea to break a lot of existing (but maybe suboptimal) code by introducing this rather surprising behaviour. I think perfect forwarding for signal parameters, at least how it is currently implemented, is not a good idea. I think for now I will revert perfect forwarding, and add those tests to our test suite to prevent those kinds of regressions in the future. Maybe with some added constraints it can be done, but I think it's a low priority thing.
Updated by Bruce Toll almost 4 years ago
Thanks for the detailed follow-up and test case. I very much agree with your decision.
Updated by Roel Standaert almost 4 years ago
- Status changed from New to Closed
Ok, I added the test cases and reverted the changes. Closing this since it was never in a released version of Wt anyway.