| < Previous by Date | Date Index | Next by Date > |
| < Previous in Thread | Thread Index |
|
I removed the offending log statement; I
will re-work the code so that the delete this is removed and ConnectionBase
lifetime is more explicit. Joszef wrote some testes in tfm for this; they currently
are expected to fail; will look into changing this. --Derek From:
resiprocate-devel-bounces@xxxxxxxxxxxxxxxxxxx
[mailto:resiprocate-devel-bounces@xxxxxxxxxxxxxxxxxxx] On Behalf Of Byron Campen Huh,
this is a behavior I was not aware of. Makes more sense than the alternative
though. Sorry for the false alarm. Best regards, Byron Campen
In your example the
Connection destructor should get properly called right? BTW: I found the
following text: Note: if your base class
has a virtual destructor,
then your destructor is automatically virtual. You might need an explicit destructor for other reasons,
but there's no need to redeclare a destructor simply to make sure it is virtual. No matter whether you declare it with the virtual keyword, declare it without the virtual keyword, or don't declare it at all, it's still virtual. At: http://www.parashift.com/c++-faq-lite/virtual-functions.html#faq-20.7 Scott From: Byron Campen [mailto:bcampen@xxxxxxxxxxxx] Scott:
Yes, but all destructors in a class tree need to be virtual, or you run into
problems. For instance, if you have a TcpConnection* cast as a Connection*, and
you delete it, ~TcpConnection() never gets called, since the vptr doesn't exist. list:
I would argue that this self-deletion behavior is incorrect, and needs to be
removed. Having "delete this" as a side-effect of a call is usually
insane, and makes a class unsuitable for stack allocation. (I am planning on
making a sweep through the codebase for other instances of "delete
this", and bringing poorly-chosen instances to the attention to the list)
Also, this sort of decision should not be made way down at the base-class; this
should be up to the Transport objects. Best regards, Byron Campen
I suspect what is causing
the trap is the DebugLog of *currConnection after it has been deleted – we
could either remove this log, or to get rid of the self deletion… Perhaps
we should change preparseNewBytes to return an error (or set to a newly created
error state) – then in Connection::read(), if preparseNewBytes returns an error
– we should return bytesRead < 0 – which will cause TcpBaseTransport to
delete the connection. As for the destructor
problem – you are correct – default destructors are not virtual. But
TlsConnection is not inherited from TcpConnection – it is derived from
Connection – which does have a virtual destructor. Scott From: resiprocate-devel-bounces@xxxxxxxxxxxxxxxxxxx
[mailto:resiprocate-devel-bounces@xxxxxxxxxxxxxxxxxxx]
On Behalf Of Byron Campen
Here is the relevant code in TcpBaseTransport... *snip* Connection* currConnection =
mConnectionManager.getNextRead(fdset); if (currConnection) { if ( fdset.readyToRead(currConnection->getSocket())
||
currConnection->hasDataToRead() ) { DebugLog
(<< "TcpBaseTransport::processSomeReads() " <<
*currConnection);
fdset.clear(currConnection->getSocket());
int bytesRead = currConnection->read(mStateMachineFifo); DebugLog
(<< "TcpBaseTransport::processSomeReads() "
<< *currConnection << "
read=" << bytesRead); if
(bytesRead < 0) {
DebugLog (<< "Closing connection bytesRead=" <<
bytesRead);
delete currConnection; } } *snip* The problem here is that
Connection::read() can result in deletion of currConnection (see the following) from Connection::read() *snip* int Connection::read(Fifo<TransactionMessage>&
fifo) { std::pair<char*,
size_t> writePair = getWriteBuffer(); size_t bytesToRead =
resipMin(writePair.second,
static_cast<size_t>(Connection::ChunkSize)); assert(bytesToRead > 0); int bytesRead =
read(writePair.first, bytesToRead); if (bytesRead <= 0) { delete []
writePair.first; return bytesRead; }
getConnectionManager().touch(this);
preparseNewBytes(bytesRead, fifo); //.dcm. may delete this return bytesRead; } *snip* from ConnectionBase::preparseNewBytes() *snip* case
ReadingHeaders: { unsigned
int chunkLength = mBufferPos + bytesRead; char
*unprocessedCharPtr;
MsgHeaderScanner::ScanChunkResult scanChunkResult =
mMsgHeaderScanner.scanChunk(mBuffer,
chunkLength,
&unprocessedCharPtr); if
(scanChunkResult == MsgHeaderScanner::scrError) {
//.jacob. Not a terribly informative warning.
WarningLog(<< "Discarding preparse!");
delete [] mBuffer;
mBuffer = 0;
delete mMessage;
mMessage = 0;
//.jacob. Shouldn't the state also be set here?
delete this;
return; } *snip*
Long story short, someone throws garbage at us over a Tcp socket, and we
corrupt our heap. So, where should the responsibility for deletion of
connections lie? If it belongs in ConnectionBase, we need to document this in
the header file, in upper case, preferably about 50 times. (Although it looks
like the function return cannot be used to discover whether or not deletion
occurred, and it appears that this would be difficult to change, so I would
argue we need to stop this behavior.) And, as a general principle, I get very angry when my objects delete
themselves on a whim. Imagine the fun if I had a stack-allocated Connection
that decided to delete itself?
Also, it is worth noting that a virtual destructor has not been declared for
TcpConnection, and an explicitly non-virtual destructor has been declared for
TlsConnection. I am not sure whether auto-generated destructors are virtual, so
there are potentially two more guns aimed at our foot here. Best regards, Byron Campen
|