Skip to content

[Issue #153] Fix handler memory leak#154

Merged
merlimat merged 6 commits intoapache:masterfrom
freeznet:fix-handler-memory-leak
Jan 16, 2020
Merged

[Issue #153] Fix handler memory leak#154
merlimat merged 6 commits intoapache:masterfrom
freeznet:fix-handler-memory-leak

Conversation

@freeznet
Copy link
Contributor

@freeznet freeznet commented Dec 31, 2019

Fixes: #153

Makes producer & consumer removed from client handler when producer / consumer closed.

@cckellogg
Copy link
Contributor

cckellogg commented Dec 31, 2019

Can you add a test for this?

cckellogg
cckellogg reviewed Dec 31, 2019
View reviewed changes
@jiazhai
Copy link
Member

jiazhai commented Jan 10, 2020

@freeznet Would you please help handle @cckellogg 's comments? Thanks

@freeznet freeznet force-pushed the fix-handler-memory-leak branch from ab6f5a9 to 0a63338 Compare January 13, 2020 08:04
@freeznet
Copy link
Contributor Author

freeznet commented Jan 13, 2020

@cckellogg deadlock has been resolved as you mentioned, also add test for Del. PTAL again, thanks.

@freeznet freeznet force-pushed the fix-handler-memory-leak branch from 0a63338 to 7c0563f Compare January 13, 2020 08:35
cckellogg
cckellogg reviewed Jan 13, 2020
View reviewed changes
Copy link
Contributor

@cckellogg cckellogg left a comment

Choose a reason for hiding this comment

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

It seems like the tests did not run on the last commit?

pulsar/consumer_impl.go Outdated
wg.Wait()
close(c.closeCh)
})
c.client.handlers.Del(c)
Copy link
Contributor

@cckellogg cckellogg Jan 13, 2020

Choose a reason for hiding this comment

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

Is there a reason not to call this in the closeOnce?

Copy link
Contributor Author

@freeznet freeznet Jan 14, 2020

Choose a reason for hiding this comment

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

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.

@freeznet
Copy link
Contributor Author

freeznet commented Jan 14, 2020

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

@freeznet freeznet force-pushed the fix-handler-memory-leak branch from 7c0563f to eb7ce1e Compare January 14, 2020 02:22
@cckellogg
Copy link
Contributor

cckellogg commented Jan 16, 2020

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.

@merlimat
Copy link
Contributor

merlimat commented Jan 16, 2020

retest this please

@merlimat merlimat merged commit 4d788d9 into apache:master Jan 16, 2020
@wolfstudy wolfstudy added this to the 0.1.0 milestone Mar 23, 2020
@nickmurr nickmurr mentioned this pull request Aug 21, 2020
Closed
@sijie sijie mentioned this pull request Aug 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cckellogg cckellogg cckellogg left review comments

@merlimat merlimat merlimat approved these changes

Assignees

No one assigned

Labels

type/bug

Projects

None yet

Milestone

0.1.0

Development

Successfully merging this pull request may close these issues.

Memory leaks when producer/consumer close

5 participants

@freeznet @cckellogg @jiazhai @merlimat @wolfstudy