Skip to content

DbgTransportSession: delete message copy when not queued #50550

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
1 commit merged into from
May 24, 2021
Merged

DbgTransportSession: delete message copy when not queued #50550

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 19 additions & 17 deletions src/coreclr/debug/shared/dbgtransportsession.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,13 @@ HRESULT DbgTransportSession::SendMessage(Message *pMessage, bool fWaitsForReply)
// queue).
pMessage->m_sHeader.m_dwLastSeenId = m_dwLastMessageIdSeen;

// Check the session state.
if (m_eState == SS_Closed)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved this check up, so we don't create a message copy to delete it.

{
// SS_Closed is bad news, we'll never recover from that so error the send immediately.
return E_ABORT;
}

// If the caller isn't waiting around for a reply we must make a copy of the message to place on the
// send queue.
pMessage->m_pOrigMessage = pMessage;
Expand Down Expand Up @@ -668,16 +675,14 @@ HRESULT DbgTransportSession::SendMessage(Message *pMessage, bool fWaitsForReply)
pMessage = pMessageCopy;
}

// Check the session state.
if (m_eState == SS_Closed)
// If the state is SS_Open we can send the message now.
if (m_eState == SS_Open)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This send was moved before the queuing of the message. I assume this is not a problem because we're holding a state lock.

{
// SS_Closed is bad news, we'll never recover from that so error the send immediately.
if (pMessageCopy)
delete pMessageCopy;
if (pDataBlockCopy)
delete [] pDataBlockCopy;

return E_ABORT;
// Send the message header block followed by the data block if it's provided. Any network error will
// be reported internally by SendBlock and result in a transition to the SS_Resync_NC state (and an
// eventual resend of the data).
if (SendBlock((PBYTE)&pMessage->m_sHeader, sizeof(MessageHeader)) && pMessage->m_pbDataBlock)
SendBlock(pMessage->m_pbDataBlock, pMessage->m_cbDataBlock);
}

// Don't queue session management messages. We always recreate these if we need to re-send them.
Expand All @@ -700,15 +705,12 @@ HRESULT DbgTransportSession::SendMessage(Message *pMessage, bool fWaitsForReply)
pMessage->m_pNext = NULL;
}
}

// If the state is SS_Open we can send the message now.
if (m_eState == SS_Open)
else
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, we're either adding the message to the queue, where the queue reader is responsible for freeing.

Or else we delete when we've allocated a copy.

{
// Send the message header block followed by the data block if it's provided. Any network error will
// be reported internally by SendBlock and result in a transition to the SS_Resync_NC state (and an
// eventual resend of the data).
if (SendBlock((PBYTE)&pMessage->m_sHeader, sizeof(MessageHeader)) && pMessage->m_pbDataBlock)
SendBlock(pMessage->m_pbDataBlock, pMessage->m_cbDataBlock);
if (pMessageCopy)
delete pMessageCopy;
if (pDataBlockCopy)
delete [] pDataBlockCopy;
}

// If the state wasn't open there's nothing more to be done. The state will eventually transition to
Expand Down