Compiler warning when "delete"ing a client instance

[fredlcore]

The following code results in the warning below:

max_cul = new WiFiSpiClient();
[...]
delete max_cul;
max_cul = NULL;

warning: deleting object of polymorphic class type 'WiFiSpiClient' which has non-virtual destructor might cause undefined behavior [-Wdelete-non-virtual-dtor]
     delete max_cul;

Is there a way to prevent this warning?

[JiriBilek]

It seems to me an explicit virtual destructor is needed. The destructor should close the connection and free the socket, if used.
I'll fix it ASAP.
Thanks.

[fredlcore]

That would be great, I just added an empty function

WiFiSpiClient::~WiFiSpiClient() {
}

in WiFiSpiClient.cpp as well as

    virtual ~WiFiSpiClient();

in WiFiSpiClient.h and it prevents the warning (not sure if "virtual" is correct here), but you are right, the connection should be closed and the socket freed as well. Thanks for looking into this, looking for the patch!

[JAndrassy]

It seems to me an explicit virtual destructor is needed. The destructor should close the connection and free the socket, if used.
I'll fix it ASAP.
Thanks.

no. the WiFiClient of Arduino net API should not close the connection if destroyed. (server.available() returns a copy, some function can take it in parameter as a copy)

WiFiClient should be only a pointer to internal representation. In ESP8266WiFi library the WiFiClient does it with ClientContext. All objects of the WiFiClient class for the same TCP connection must see the same state of the connection including for example buffer and count of data available.

I recently found out that the WiFiClientSecure for BearSSL in esp8266 WiFi library doesn't work on this principle and the developers fixed it immediately with a not trivial change.

In this library the WiFiClient should only point to WiFiClient object on firmware side as it is in Ethernet library and other WiFi libraries which are only remote API of firmware.

[JiriBilek]

Thanks Juraj!
I see, the problem is really in the server code, thanks for mentioning it. Closing the connection when destroying the WiFiClient object would hopefully be ok and desirable when using ESP as a web client but disastrous when using it as a server.

WiFiSpi/src/WiFiSpiServer.cpp

Line 62 in ec4a9d4

WiFiSpiClient client(_sock);

But it seems to me, there should be a housekeeping after destroying the WiFiClient that is not linked to the server socket as there would be no chance to connect to the socket and close it afterwards.

[fredlcore]

Thanks, that seems to correspond with what I found here:
https://stackoverflow.com/questions/12994920/how-to-delete-an-object-of-a-polymorphic-class-type-that-has-no-virtual-destruct

Does "server code" here mean the "WiFiSpiEsp" code/library? Or code inside the ESP8266-SDK?
In the meantime, should I then refrain from using "delete" or just ignore the warning if the necessary changes are not trivial and will take some time?

[JiriBilek]

The library can be used to run a TCP client or server.
The server opens a listener and every time a connection is made it creates a WiFiClient object. This object is not supposed to free the socket in the WiFiSpiClass::_state array because it serves for subsequent requests.
On the other hand, I think, closing the connection when WiFiClient goes out of scope or is deleted is perfectly fine for client connection (imagine e.g. sending data to a web service using REST API). I have always used the WiFiClient object as static object, so I did not have the problem. I'll fix it once I make a test case.

[JAndrassy]

Jiri, this is my PR to esp8266 WiFi library, which contains a discussion about correct Arduino Server and Client implementation
esp8266/Arduino#7612

[JiriBilek]

Juraj, the problem with multiple WiFiClient objects for one connection is constrained to TCP server functionality, right?
I admit, I am a little confused, must study some code and refresh my code as well.

[JAndrassy]

it is valid for arduino code to copy the Ethernet/WiFiClient object. as assignment or as function parameter by copy (parameter type without & or *). this is not (ab)used widely, but can occur. I can't now find an example. I checked some libraries which work with Client and they all use Client&.
This is similar how String is used.

String someFns(String s) {
  String s2 = s;
  return s2;
}

is valid and works. it looks crazy for a C++ programmer and is not efficient (compiler optimizes it to some level)

[JiriBilek]

I see. The problem is quite bigger than I originally thought.
Even in my code, the implicit copy constructor is used (on return from the function):

WiFiSpiClient WiFiSpiServer::available(uint8_t* status)
{
    WiFiSpiClient client(_sock);

    uint8_t _client_status = client.status();  // creates Client object on ESP side if there is established connection
//    uint8_t _server_status = this->status();  removed, may be related with the comment below, running fine without it 

    if (status != NULL)
        *status = _client_status;

    // TODO: If server is not in listen state, restart it (code present in original WiFiServer.h). I think it is useless.

    if (_client_status == ESTABLISHED)
        return client;

    return WiFiSpiClient(SOCK_NOT_AVAIL);  // closed Client
}

Definitely, WiFiClient destructor should not close the connection.
Thanks for the help.

[JiriBilek]

index to an array on the ESP side. See https://github.com/JiriBilek/WiFiSpiESP/blob/018349de89ef63ff8f4d01792f9bfda29b4c9eca/WiFiSPIESP/WiFiSPICmd.h#L39 and next few lines

It must be synchronized between the protocol (SPI) client and server.

Edit: maybe you are asking the meaning of the command WiFiSpiClient client(_sock); It creates the client object for the given server one.

[JAndrassy]

sorry I deleted the comment to not open a new problem. I don't want to look into source code of your library now, but shouldn't the firmware return 'sock' index of the current client. there can be more then one client connected.

and next problem is that, the esp8266 WiFiServer available() implementation doesn't comply with Arduino.
https://www.arduino.cc/en/Reference/WiFiServerAvailable

[JiriBilek]

@fredlcore : weird thing: the following code compiles without error. Am I missing something?

Compiled with arm-none-eabi-gcc\4.8.3-2014q1

#include "WiFiSpiClient.h"

void setup() {
    WiFiSpiClient *pp = new WiFiSpiClient();
    delete pp;
}

void loop() {
}

[fredlcore]

Ah, ok! The difference to my code is that I don't have the *.
@dukess: Is there a reason why you didn't use a pointer here, or can we change it without any other unintended effects?

[dukess]

Reason is only one: pointer required only 2(4) bytes RAM when we won't need network connection. As usually it is not life critical for Due but for Mega.

In our code we use pointer.

#ifdef MAX_CUL
#ifdef WIFI
WiFiSpiClient *max_cul;
#else
EthernetClient *max_cul;
#endif
#endif

[JAndrassy]

Ah, ok! The difference to my code is that I don't have the *.
@dukess: Is there a reason why you didn't use a pointer here, or can we change it without any other unintended effects?

you have a pointer. max_cul is a pointer. it would not compile if it wouldn't

@dukess, repeated heap allocation fragments the heap and there is no memory management to defragment it.

[fredlcore]

Sorry, I looked further down in our code where it says
max_cul = new WiFiSpiClient();
but of course max_cul was defined earlier as you point out correctly.

@JiriBilek: Hm, if you had a gcc version prior to 4.7, the warning was apparently not shown, see here:
https://lists.boost.org/Archives/boost/2012/02/190544.php
But with your compiler version, it therefore should - unless arm's gcc is still behind on that one?

[fredlcore]

...and yes, that seems to be the case: If I compile your code snippet, I get the following warning:

/Users/frederik/Documents/PlatformIO/Projects/BSB-LAN/src/BSB_lan.ino: In function 'void setup()':
/Users/frederik/Documents/PlatformIO/Projects/BSB-LAN/src/BSB_lan.ino:5:12: warning: deleting object of polymorphic class type 'WiFiSpiClient' which has non-virtual destructor might cause undefined behavior [-Wdelete-non-virtual-dtor]
     delete pp;
            ^~

[JiriBilek]

@fredlcore Now, I tested compilation under xpack-arm-none-eabi-gcc/9.2.1-1.1, got warning about deprecated boolean type but nothing more. Could you please post your compiler version?

[fredlcore]

arm-none-eabi-g++ (GNU Tools for Arm Embedded Processors 7-2017-q4-major) 7.2.1 20170904 (release) [ARM/embedded-7-branch revision 255204]
Strange that your newer compiler version would not complain about this, but my older one would?
Have you tried with -Wdelete-non-virtual-dtor?

[JiriBilek]

With -Wdelete-non-virtual-dtor I get the warning (on 9.2.1), thanks.

[fredlcore]

I'm nowhere near being a C++ guru, but maybe this is helpful?
https://stackoverflow.com/questions/43282826/suppress-delete-non-virtual-dtor-warning-when-using-a-protected-non-virtual-dest
The selected answer mentions several ways to deal with this, but my OO-related knowledge is too limited to tell which one would be the most suitable here.
It also mentions the two different error messages where one says "will cause undefined behaviour" and one says "might cause undefined behaviour". We're in the "might" area here, so maybe the problem will only become manifest if we were to inherit from WiFiSpiClient. But still it would probably be good to adopt one of the approaches mentioned there to prevent any possible undefined behaviour?

[JiriBilek]

Fixed: 28f491e

[fredlcore]

Great, thanks!

[dukess]

@JAndrassy Yesterday I tried turning the connection on and off multiple times to see if there would be memory leaks. Maybe I was lucky, but the size of free memory did not decrease.

[dukess]

@JiriBilek thank you!

[JiriBilek]

@ everybody: you're welcome

@dukess It's great you have no problems in heap fragmentation.
Nevertheless, Juraj is right, in general. If you are RAM constrained, you should not repeatedly allocate big objects on the heap. I learned this in automotive applications, where you'd agree, the reliability is more than desired. Some practices forbid using heap at all, another allow heap usage but you must do all your allocation in boot time of the app.
I haven't measured it, but WiFiClient class should be not big, so the risk of heap fragmentation should be low.

[dukess]

@JiriBilek Thanks for the explanation. I think that in our project, we should not be too afraid of fragmentation: configuration changes (enabling/disabling modules) are rare, and we can send the controller to restart after saving the configuration. The main purpose of using pointers is to save memory when the module is not in use.

[fredlcore]

BTW: the same warning also occurs with the official Ethernet 2.0 library...

[dukess]

We have two way: add destructor independently or ask developers.

[fredlcore]

I opened a pull request, but this is no longer the scope of this issue here, just wanted to mention that this seems to be a more general problem...