You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/04/26 17:02:41 UTC

[GitHub] [geode-native] pdxcodemonkey opened a new pull request #792: GEODE-9193: Use static object for PING messages.

pdxcodemonkey opened a new pull request #792:
URL: https://github.com/apache/geode-native/pull/792


   
   - Prevents a leak at shutdown, ensures object is properly destroyed.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode-native] mmartell commented on a change in pull request #792: GEODE-9193: Use local object variable for PING messages.

Posted by GitBox <gi...@apache.org>.
mmartell commented on a change in pull request #792:
URL: https://github.com/apache/geode-native/pull/792#discussion_r622287890



##########
File path: cppcache/src/TcrEndpoint.cpp
##########
@@ -504,14 +504,15 @@ void TcrEndpoint::pingServer(ThinClientPoolDM* poolDM) {
   }
 
   if (!m_msgSent && !m_pingSent) {
-    TcrMessagePing* pingMsg = TcrMessage::getPingMessage(m_cacheImpl);
+    TcrMessagePing pingMsg(new DataOutput(m_cacheImpl->createDataOutput()),

Review comment:
       Where is DataOutput deleted? Hopefully, that's not also a leak.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode-native] pdxcodemonkey merged pull request #792: GEODE-9193: Use local object variable for PING messages.

Posted by GitBox <gi...@apache.org>.
pdxcodemonkey merged pull request #792:
URL: https://github.com/apache/geode-native/pull/792


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #792: GEODE-9193: Use local object variable for PING messages.

Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #792:
URL: https://github.com/apache/geode-native/pull/792#discussion_r622495428



##########
File path: cppcache/src/TcrEndpoint.cpp
##########
@@ -504,14 +504,15 @@ void TcrEndpoint::pingServer(ThinClientPoolDM* poolDM) {
   }
 
   if (!m_msgSent && !m_pingSent) {
-    TcrMessagePing* pingMsg = TcrMessage::getPingMessage(m_cacheImpl);
+    TcrMessagePing pingMsg(new DataOutput(m_cacheImpl->createDataOutput()),
+                           true);

Review comment:
       If this value is always `true` then let's just roll it into the constructor initializer and remove from parameters.

##########
File path: cppcache/src/TcrEndpoint.cpp
##########
@@ -504,14 +504,15 @@ void TcrEndpoint::pingServer(ThinClientPoolDM* poolDM) {
   }
 
   if (!m_msgSent && !m_pingSent) {
-    TcrMessagePing* pingMsg = TcrMessage::getPingMessage(m_cacheImpl);
+    TcrMessagePing pingMsg(new DataOutput(m_cacheImpl->createDataOutput()),

Review comment:
       Can you copy my PR where I modified the constructor to take a `std::unique_ptr<DataOutput>` to avoid any leaking potentials in allocating here and failing to take ownership in the constructor. Also makes it more clean who owns it. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on pull request #792: GEODE-9193: Use static object for PING messages.

Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #792:
URL: https://github.com/apache/geode-native/pull/792#issuecomment-827748183


   Converting to draft for the moment, trying to nail down what's going on with CI failures.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode-native] pdxcodemonkey commented on a change in pull request #792: GEODE-9193: Use local object variable for PING messages.

Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #792:
URL: https://github.com/apache/geode-native/pull/792#discussion_r622441292



##########
File path: cppcache/src/TcrEndpoint.cpp
##########
@@ -504,14 +504,15 @@ void TcrEndpoint::pingServer(ThinClientPoolDM* poolDM) {
   }
 
   if (!m_msgSent && !m_pingSent) {
-    TcrMessagePing* pingMsg = TcrMessage::getPingMessage(m_cacheImpl);
+    TcrMessagePing pingMsg(new DataOutput(m_cacheImpl->createDataOutput()),

Review comment:
       Yeah - it's not pretty, but it is correct.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #792: GEODE-9193: Use local object variable for PING messages.

Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #792:
URL: https://github.com/apache/geode-native/pull/792#discussion_r622427386



##########
File path: cppcache/src/TcrEndpoint.cpp
##########
@@ -504,14 +504,15 @@ void TcrEndpoint::pingServer(ThinClientPoolDM* poolDM) {
   }
 
   if (!m_msgSent && !m_pingSent) {
-    TcrMessagePing* pingMsg = TcrMessage::getPingMessage(m_cacheImpl);
+    TcrMessagePing pingMsg(new DataOutput(m_cacheImpl->createDataOutput()),

Review comment:
       This pointer ownership is transfered to the TcrMessage by assigning it to an unique_ptr. I am with you that the way of instantiating the object might lead to think that this is a memory leak, but this is they way is done for most of the TcrMessage implementations :S




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org