Fix issue #5507 "Constructor WebServer::WebServer(IPAddress addr, int… #5509
Conversation
…addr, int port) produces an unexpected result" "The class Webserver is declared with two explicit constructors, one with signature: WebServer::WebServer(IPAddress addr, int port) Using this results in a server listening on the port number obtained by converting the value of the IPAddress addr argument (in host byte order) to a uint32_t and then to a uint16_t, which is manifestly not the result that would be expected. ... As for a fix, we can assume from these results that this constructor is not being used and therefore could simply be deleted."
|
Wouldn't it be better to implement the missing function like in old esp: Furthermore removing maybe break derived classes.. so better fix |
|
I agree with @Rotzbua . Issue should be fixed in |
I would readily agree if there was evidence that the ESP8266 version was in use and known to work and we were able to properly test an ESP32 version. I can't see any examples (ESP8266 or ESP32) that use it and note that a server binding a specific IP address (as opposed to INADDR_ANY) is really only useful, and can only be tested, if you have a device with multiple network interfaces, e.g. one WiFi plus one ETH. |
|
@timr49 even though there are no examples, in reality we can/should add this functionality, because ESP-IDF supports it and could be useful in some cases. ESP32 can have more than 3 interfaces at the same time :) For now the code can just go to the other constructor. |
Reverse changes in commit bee1e70 after discussion. Alternative fix to be done.
It occurred to me after commenting above that the ESP WiFi can have two IP interfaces, one the station and one the soft AP. So, we do have a way of testing multi-homed servers, at least for n=2. |
…addr, int port) produces an unexpected result" This change adds support for multi-homed servers to libraries/WiFi. It was assumed to be there already by libraries/WebServer, but was not. This led to unexpected results when the IP address-specific constructor of class WebServer was used (see issue 5507). This change was tested using three concurrent instances of WebServer, one bound to the WiFi station address, one bound to the WiFi soft AP address, and one bound to INADDR_ANY. See libraries/WebServer/examples/MultiHomedServers for the test method.
|
I have pushed a different change onto the same branch, which adds support for multi-homed web servers. It includes a new examples script (WebServer/MultiHomedServers), which tests this change. The testing method is in the comments. The expected results were achieved. At about the same time as my push, I got some error notifications from CI, but am not sure whether they are related to my previous fix or this one. Unfortunately, my GitHub skills are new, so am not sure what to do next. |
…addr, int port) produces an unexpected result" (cont.) This fixes what I think might be the cause of CI failures on GitHub for the previous commit, namely the absence of an include file in examples/MultiHomedServers.
I successfully guessed what I had done wrong and fixed it in a further commit to the pull request branch ("All checks have passed"). I believe that this change is now ready for review. |
|
|
||
| server0 = new WebServer(8080); | ||
| server1 = new WebServer(WiFi.localIP(), 8081); | ||
| server2 = new WebServer(WiFi.softAPIP(), 8082); |
There was a problem hiding this comment.
for this test to be valid, both servers should be on the same port. That is how you can make sure that the functionality works.
There was a problem hiding this comment.
Changed port numbers as suggested. Expected results returned by test script. Pushed new commit. "All checks have passed".
…addr, int port) produces an unexpected result" (cont.) Change port numbers in examples/MultiHomedServers per pull-request comment from me-no-dev ... "for this test to be valid, both servers should be on the same port. That is how you can make sure that the functionality works."
|
Thanks @timr49 !!! all merged :) |
… port) produces an unexpected result"
"The class Webserver is declared with two explicit constructors, one with signature:
WebServer::WebServer(IPAddress addr, int port)
Using this results in a server listening on the port number obtained by converting the value of the IPAddress addr argument (in host byte order) to a uint32_t and then to a uint16_t, which is manifestly not the result that would be expected.
...
As for a fix, we can assume from these results that this constructor is not being used and therefore could simply be deleted."
This entire section can be deleted if all items are checked.
By completing this PR sufficiently, you help us to improve the quality of Release Notes
Checklist
Summary
Fix issue #5507 "Constructor WebServer::WebServer(IPAddress addr, int port) produces an unexpected result"
Impact
This change deletes the declaration and invalid definition of an apparently unused constructor of class WebServer. The intended/expected impact is that (attempted) use of this constructor will result in a compile-time error rather than erroneous run-time behaviour.