You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/10/21 09:25:47 UTC
[GitHub] [pulsar] bschofield opened a new pull request #8325: [8311][pulsar-client-go] Fix memory leak in cgo golang client
bschofield opened a new pull request #8325:
URL: https://github.com/apache/pulsar/pull/8325
Fixes #8311.
### Motivation
In the C/C++ glue code, file *c_Producer.cc* function *handle_producer_send()*, a new `pulsar_message_id_t` is created. This needs to be freed at some point.
Because of issues with legacy C clients, it is not possible to call *delete* from inside `c_Producer.cc` (see commentary on #8311). Instead, the client must call *pulsar_message_id_free()*.
Previously, the cgo interface was not freeing this message ID, which caused a memory leak on every message sent.
### Modifications
This PR adds the missing calls to *pulsar_message_id_free()*. The calls are added in the cgo layer, in *pulsarProducerSendCallbackProxy()* and *pulsarProducerSendCallbackProxyWithMsgID()*.
### Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
### Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API: no
- The schema: no
- The default values of configurations: no
- The wire protocol: no
- The rest endpoints: no
- The admin cli options: no
- Anything that affects deployment: no
### Documentation
- Does this pull request introduce a new feature? no
----------------------------------------------------------------
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] [pulsar] bschofield commented on a change in pull request #8325: [Issue 8311][pulsar-client-go] Fix memory leak in cgo golang client
Posted by GitBox <gi...@apache.org>.
bschofield commented on a change in pull request #8325:
URL: https://github.com/apache/pulsar/pull/8325#discussion_r510322142
##########
File path: pulsar-client-go/pulsar/c_producer.go
##########
@@ -298,6 +298,7 @@ func pulsarProducerSendCallbackProxyWithMsgID(res C.pulsar_result, messageId *C.
sendCallback.callback(getMessageId(messageId), newError(res, "Failed to send message"))
} else {
sendCallback.callback(getMessageId(messageId), nil)
+ C.pulsar_message_id_free(messageId)
Review comment:
You're absolutely right. I didn't even think to check that getter for side effects. Removed in latest commit. Thanks!
----------------------------------------------------------------
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] [pulsar] merlimat commented on a change in pull request #8325: [Issue 8311][pulsar-client-go] Fix memory leak in cgo golang client
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #8325:
URL: https://github.com/apache/pulsar/pull/8325#discussion_r510272691
##########
File path: pulsar-client-go/pulsar/c_producer.go
##########
@@ -298,6 +298,7 @@ func pulsarProducerSendCallbackProxyWithMsgID(res C.pulsar_result, messageId *C.
sendCallback.callback(getMessageId(messageId), newError(res, "Failed to send message"))
} else {
sendCallback.callback(getMessageId(messageId), nil)
+ C.pulsar_message_id_free(messageId)
Review comment:
@bschofield I think this free should not be done, because `getMessageId(messageId)` is already attaching a finalizer to the Go object and it will free the message id automatically:
```
func getMessageId(messageId *C.pulsar_message_id_t) MessageID {
msgId := &messageID{ptr: messageId}
runtime.SetFinalizer(msgId, messageIdFinalizer)
return msgId
}
```
##########
File path: pulsar-client-go/pulsar/c_producer.go
##########
@@ -298,6 +298,7 @@ func pulsarProducerSendCallbackProxyWithMsgID(res C.pulsar_result, messageId *C.
sendCallback.callback(getMessageId(messageId), newError(res, "Failed to send message"))
} else {
sendCallback.callback(getMessageId(messageId), nil)
+ C.pulsar_message_id_free(messageId)
Review comment:
@bschofield I think this free should not be done, because `getMessageId(messageId)` is already attaching a finalizer to the Go object and it will free the message id automatically:
```go
func getMessageId(messageId *C.pulsar_message_id_t) MessageID {
msgId := &messageID{ptr: messageId}
runtime.SetFinalizer(msgId, messageIdFinalizer)
return msgId
}
```
----------------------------------------------------------------
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] [pulsar] wolfstudy commented on pull request #8325: [Issue 8311][pulsar-client-go] Fix memory leak in cgo golang client
Posted by GitBox <gi...@apache.org>.
wolfstudy commented on pull request #8325:
URL: https://github.com/apache/pulsar/pull/8325#issuecomment-716952266
/pulsarbot run-failure-checks
----------------------------------------------------------------
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] [pulsar] bschofield commented on a change in pull request #8325: [Issue 8311][pulsar-client-go] Fix memory leak in cgo golang client
Posted by GitBox <gi...@apache.org>.
bschofield commented on a change in pull request #8325:
URL: https://github.com/apache/pulsar/pull/8325#discussion_r509515848
##########
File path: pulsar-client-go/pulsar/c_go_pulsar.h
##########
@@ -61,7 +61,7 @@ static inline void _pulsar_producer_close_async(pulsar_producer_t *producer, voi
pulsar_producer_close_async(producer, pulsarProducerCloseCallbackProxy, ctx);
}
-void pulsarProducerSendCallbackProxy(pulsar_result result, pulsar_message_id_t *message, void *ctx);
+void pulsarProducerSendCallbackProxy(pulsar_result result, pulsar_message_id_t *messageId, void *ctx);
Review comment:
This doesn't change anything, it's just for consistency with the next line.
----------------------------------------------------------------
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] [pulsar] wolfstudy merged pull request #8325: [Issue 8311][pulsar-client-go] Fix memory leak in cgo golang client
Posted by GitBox <gi...@apache.org>.
wolfstudy merged pull request #8325:
URL: https://github.com/apache/pulsar/pull/8325
----------------------------------------------------------------
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