Skip to content

Comments

Dereference device with a defer call to prevent memory leaks#98

Merged
zagrodzki merged 3 commits intogoogle:masterfrom
danielpaulus:fix-memleak-in-opendevices
Nov 1, 2021
Merged

Dereference device with a defer call to prevent memory leaks#98
zagrodzki merged 3 commits intogoogle:masterfrom
danielpaulus:fix-memleak-in-opendevices

Conversation

@danielpaulus
Copy link
Contributor

This PR fixes the memory leak mentioned in #97

gousb.OpenDevices currently only dereferences libusb device pointers when opening is unsuccessful or when the opener returns false.
This can cause a memory leak that becomes mostly visible when using multiple libusb contexts in a long running application.

The fix is to always call c.libusb.dereference(dev) so the device reference counter is decreased when OpenDevices is done. That is not a problem as libusb increases the reference counter internally on the c.libusb.open(dev) call and releases it when we call c.libusb.close(dev) again.

…n internal reference so there is no risk of breaking anything.
@google-cla
Copy link

google-cla bot commented Apr 23, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 79.409% when pulling e40a201 on danielpaulus:fix-memleak-in-opendevices into 0eba1b1 on google:master.

@coveralls
Copy link

coveralls commented Apr 23, 2021

Coverage Status

Coverage increased (+0.2%) to 79.431% when pulling 9cc7659 on danielpaulus:fix-memleak-in-opendevices into 0eba1b1 on google:master.

@danielpaulus
Copy link
Contributor Author

@googlebot I signed it!

@danielpaulus
Copy link
Contributor Author

Coverage Status

Coverage increased (+0.1%) to 79.409% when pulling e40a201 on danielpaulus:fix-memleak-in-opendevices into 0eba1b1 on google:master.

wow I even increased code coverage ;-)

Copy link
Collaborator

@zagrodzki zagrodzki left a comment

Choose a reason for hiding this comment

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

could you also update a comment in https://github.com/google/gousb/blob/master/libusb.go#L212? it is misleading now as it is, since it implies that all devices returned on the list will be "closed" (and so also opened) at some point, but this is not the case.

@danielpaulus
Copy link
Contributor Author

could you also update a comment in https://github.com/google/gousb/blob/master/libusb.go#L212? it is misleading now as it is, since it implies that all devices returned on the list will be "closed" (and so also opened) at some point, but this is not the case.

Done.
Should I also create a small unit test for OpenDevices while I am at it?

Copy link
Collaborator

@zagrodzki zagrodzki left a comment

Choose a reason for hiding this comment

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

Should I also create a small unit test for OpenDevices while I am at it?

sure, if you want. You can do that in a separate PR perhaps, this one I think is good to go.

ret = append(ret, (*libusbDevice)(d))
}
// devices will be dereferenced later, during close.
// devices must be dereferenced by the caller to prevent memoryleaks
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/memoryleaks/memory leaks

@danielpaulus
Copy link
Contributor Author

hey @zagrodzki ,
is there anything missing on the PR? If not, when do you think you can release it? I would like to move away from using my private fork :-)

@zagrodzki
Copy link
Collaborator

sorry, I assumed you will be able to merge yourself after my approval, but it seems this is not how GitHub flow works...

There is one comment left: typo in libusb.go (#98 (comment))

@zagrodzki zagrodzki merged commit 0ce3a07 into google:master Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants