search

ni / grpc-device

gRPC server providing remote access to NI device driver APIs.
MIT License
69 stars 47 forks source link

We must close the Find handle to avoid a leak. #1005

Closed dmondrik closed 11 months ago

dmondrik commented 11 months ago

What does this Pull Request accomplish?

Close the handle returned from viFindRsrc when we're done with it.

Why should this Pull Request be merged?

This fixes a resource leak.

What testing has been done?

FindRsrc_OpensResourceManagerOnceAndClosesFindHandle

dmondrik commented 11 months ago

It looks good to me. Why does FindRsrc need to open the resource handle? Does ParseRsrc open the resource handle?

Why does it need to open the resource manager? Because the first parameter to viFindRsrc is the resource manager handle. This is one of 3 operations on a resource manager. The other 2 are Parse and Open, and yes they both do the same.

danielhuani commented 11 months ago

It looks good to me. Why does FindRsrc need to open the resource handle? Does ParseRsrc open the resource handle?

Why does it need to open the resource manager? Because the first parameter to viFindRsrc is the resource manager handle. This is one of 3 operations on a resource manager. The other 2 are Parse and Open, and yes they both do the same.

I see, so it is the resource manager handle. Does Parse method need to close the resource manager as well, similar as this change to the FindRsrc?

dmondrik commented 11 months ago

I see, so it is the resource manager handle. Does Parse method need to close the resource manager as well, similar as this change to the FindRsrc?

This PR does not close the resource manager handle. This is yet another handle, specific to the response from viFindRsrc. Consider 2 distinct calls with different expressions to viFindRsrc that return multiple resources each. The "find handle" is the only way to distinguish the results, used only for repeated calls to viFindNext, and must be closed separately.

danielhuani commented 11 months ago

I see, so it is the resource manager handle. Does Parse method need to close the resource manager as well, similar as this change to the FindRsrc?

This PR does not close the resource manager handle. This is yet another handle, specific to the response from viFindRsrc. Consider 2 distinct calls with different expressions to viFindRsrc that return multiple resources each. The "find handle" is the only way to distinguish the results, used only for repeated calls to viFindNext, and must be closed separately.

Extended question not related to this PR, but why does the find handle need to be opened, and is not passed as a string instead? The resource name could be good to distinguish the resources, right?