Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent obtaining handle values from dead unique handles and stream leases. #4346

Merged
merged 4 commits into from
Oct 13, 2022

Conversation

mzient
Copy link
Contributor

@mzient mzient commented Oct 13, 2022

Signed-off-by: Michal Zientkiewicz michalz@nvidia.com

Category:

Refactoring (Redesign of existing code that doesn't affect functionality)

Description:

Prevents the API user from obtaining a dead handle value from UniqueHandle and CUDAStreamLease by deleting r-value-qualified overloads.

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

Tested locally that the misusing code doesn't compile.

  • Existing tests apply
  • New tests added
    • Python tests
    • GTests
    • Benchmark
    • Other
  • N/A

Checklist

Documentation

  • Existing documentation applies
  • Documentation updated
    • Docstring
    • Doxygen
    • RST
    • Jupyter
    • Other
  • N/A

DALI team only

Requirements

  • Implements new requirements
  • Affects existing requirements
  • N/A

REQ IDs: N/A

JIRA TASK: N/A

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@jantonguirao jantonguirao self-assigned this Oct 13, 2022
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6171060]: BUILD STARTED

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6171119]: BUILD STARTED

}

/**
* @brief Cannot obtain a valie handle from a temporary CUDAStreamLease
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @brief Cannot obtain a valie handle from a temporary CUDAStreamLease
* @brief Cannot obtain a valid handle from a temporary CUDAStreamLease

*/
constexpr handle_type get() && = delete;

/// @brief Make the wrapper usable in most context in which the handle type can be used
Copy link
Contributor

@klecki klecki Oct 13, 2022

Choose a reason for hiding this comment

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

Suggested change
/// @brief Make the wrapper usable in most context in which the handle type can be used
/**
* @brief Make the wrapper usable in most context in which the handle type can be used
*/

/**
* @brief Cannot obtain a valie handle from a temporary CUDAStreamLease
*
* By the time this function returns, the stream is returned to the pool.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder, if we are at this, we should probably mention, that the user of this API should keep the StreamLease for the duration the stream is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The class description describes the object life cycle.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6171119]: BUILD PASSED

@mzient mzient merged commit 440405e into NVIDIA:main Oct 13, 2022
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.

None yet

4 participants