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 2019/12/31 03:11:17 UTC
[GitHub] [pulsar-client-go] freeznet opened a new pull request #154: [Issue
#153] Fix handler memory leak
freeznet opened a new pull request #154: [Issue #153] Fix handler memory leak
URL: https://github.com/apache/pulsar-client-go/pull/154
Fixes: #153
Makes producer & consumer removed from client handler when producer / consumer closed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] freeznet commented on issue #154: [Issue #153]
Fix handler memory leak
Posted by GitBox <gi...@apache.org>.
freeznet commented on issue #154: [Issue #153] Fix handler memory leak
URL: https://github.com/apache/pulsar-client-go/pull/154#issuecomment-573551158
@cckellogg deadlock has been resolved as you mentioned, also add test for `Del`. PTAL again, 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
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] freeznet commented on issue #154: [Issue #153]
Fix handler memory leak
Posted by GitBox <gi...@apache.org>.
freeznet commented on issue #154: [Issue #153] Fix handler memory leak
URL: https://github.com/apache/pulsar-client-go/pull/154#issuecomment-573968255
> It seems like the tests did not run on the last commit?
@cckellogg the test runs ok on my laptop, can you provide any detail? thanks.
```
=== RUN TestClientHandlers_Del
--- PASS: TestClientHandlers_Del (0.00s)
client_handlers_test.go:61: closable1 is: closed true
client_handlers_test.go:62: closable2 is: closed true
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cckellogg commented on issue #154: [Issue #153]
Fix handler memory leak
Posted by GitBox <gi...@apache.org>.
cckellogg commented on issue #154: [Issue #153] Fix handler memory leak
URL: https://github.com/apache/pulsar-client-go/pull/154#issuecomment-575268473
What happens when you start pulsar in standalone and run
```
go test -race ./...
```
Is there a way we can trigger the unit/integration tests? It would be nice to have those run to ensure the issue has been fixed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] merlimat commented on issue #154: [Issue #153]
Fix handler memory leak
Posted by GitBox <gi...@apache.org>.
merlimat commented on issue #154: [Issue #153] Fix handler memory leak
URL: https://github.com/apache/pulsar-client-go/pull/154#issuecomment-575339815
retest this please
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cckellogg commented on a change in pull request
#154: [Issue #153] Fix handler memory leak
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #154: [Issue #153] Fix handler memory leak
URL: https://github.com/apache/pulsar-client-go/pull/154#discussion_r362293342
##########
File path: pulsar/internal/client_handlers.go
##########
@@ -36,6 +36,13 @@ func (h *ClientHandlers) Add(c Closable) {
defer h.l.Unlock()
h.handlers[c] = true
}
+
+func (h *ClientHandlers) Del(c Closable) {
+ h.l.Lock()
+ defer h.l.Unlock()
+ delete(h.handlers, c)
+}
+
Review comment:
The close method in this class will have to change otherwise there is a deadlock.
We should make a copy of the handlers then release the lock and then call close on the list of handlers. Otherwise the one handler (producer or consumer) will try to call Del() and be deadlocked.
Something like this should work
func (h *ClientHandlers) Close() {
h.l.Lock()
handlers := make([]Closable, 0, len(h.handlers))
for handler := range h.handlers {
handlers = append(handlers, handler)
}
h.l.Unlock()
for _, handler := range handlers {
handler.Close()
}
}
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] merlimat merged pull request #154: [Issue #153]
Fix handler memory leak
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #154: [Issue #153] Fix handler memory leak
URL: https://github.com/apache/pulsar-client-go/pull/154
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] freeznet commented on a change in pull request
#154: [Issue #153] Fix handler memory leak
Posted by GitBox <gi...@apache.org>.
freeznet commented on a change in pull request #154: [Issue #153] Fix handler memory leak
URL: https://github.com/apache/pulsar-client-go/pull/154#discussion_r366124434
##########
File path: pulsar/consumer_impl.go
##########
@@ -317,6 +319,7 @@ func (c *consumer) Close() {
wg.Wait()
close(c.closeCh)
})
+ c.client.handlers.Del(c)
Review comment:
the initial thought was `delete` operation will be no-op if consumer/producer not in handlers, so i didnt put `Del` call in the `closeOnce`. But your idea is better, which will prevent non necessary locks&unlocks.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] jiazhai commented on issue #154: [Issue #153]
Fix handler memory leak
Posted by GitBox <gi...@apache.org>.
jiazhai commented on issue #154: [Issue #153] Fix handler memory leak
URL: https://github.com/apache/pulsar-client-go/pull/154#issuecomment-572831217
@freeznet Would you please help handle @cckellogg 's comments? 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
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cckellogg commented on a change in pull request
#154: [Issue #153] Fix handler memory leak
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #154: [Issue #153] Fix handler memory leak
URL: https://github.com/apache/pulsar-client-go/pull/154#discussion_r366020931
##########
File path: pulsar/consumer_impl.go
##########
@@ -317,6 +319,7 @@ func (c *consumer) Close() {
wg.Wait()
close(c.closeCh)
})
+ c.client.handlers.Del(c)
Review comment:
Is there a reason not to call this in the closeOnce?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cckellogg commented on a change in pull request
#154: [Issue #153] Fix handler memory leak
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #154: [Issue #153] Fix handler memory leak
URL: https://github.com/apache/pulsar-client-go/pull/154#discussion_r362293342
##########
File path: pulsar/internal/client_handlers.go
##########
@@ -36,6 +36,13 @@ func (h *ClientHandlers) Add(c Closable) {
defer h.l.Unlock()
h.handlers[c] = true
}
+
+func (h *ClientHandlers) Del(c Closable) {
+ h.l.Lock()
+ defer h.l.Unlock()
+ delete(h.handlers, c)
+}
+
Review comment:
The close method in this class will have to change otherwise there is a deadlock.
We should make a copy of the handlers then release the lock and then call close on the list of handlers. Otherwise the one handler (producer or consumer) will try to call Del() and be deadlocked.
Something like this should work
```
func (h *ClientHandlers) Close() {
h.l.Lock()
handlers := make([]Closable, 0, len(h.handlers))
for handler := range h.handlers {
handlers = append(handlers, handler)
}
h.l.Unlock()
for _, handler := range handlers {
handler.Close()
}
}
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cckellogg commented on issue #154: [Issue #153]
Fix handler memory leak
Posted by GitBox <gi...@apache.org>.
cckellogg commented on issue #154: [Issue #153] Fix handler memory leak
URL: https://github.com/apache/pulsar-client-go/pull/154#issuecomment-570005272
Can you add a test for this?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [pulsar-client-go] cckellogg commented on a change in pull request
#154: [Issue #153] Fix handler memory leak
Posted by GitBox <gi...@apache.org>.
cckellogg commented on a change in pull request #154: [Issue #153] Fix handler memory leak
URL: https://github.com/apache/pulsar-client-go/pull/154#discussion_r362293342
##########
File path: pulsar/internal/client_handlers.go
##########
@@ -36,6 +36,13 @@ func (h *ClientHandlers) Add(c Closable) {
defer h.l.Unlock()
h.handlers[c] = true
}
+
+func (h *ClientHandlers) Del(c Closable) {
+ h.l.Lock()
+ defer h.l.Unlock()
+ delete(h.handlers, c)
+}
+
Review comment:
The close method in this class will have to change otherwise there is a deadlock.
We should make a copy of the handlers then release the lock and then call close on the list of handlers. Otherwise one of the handlers (producer or consumer) will try to call Del() and be deadlocked.
Something like this should work
```
func (h *ClientHandlers) Close() {
h.l.Lock()
handlers := make([]Closable, 0, len(h.handlers))
for handler := range h.handlers {
handlers = append(handlers, handler)
}
h.l.Unlock()
for _, handler := range handlers {
handler.Close()
}
}
```
----------------------------------------------------------------
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
With regards,
Apache Git Services