Skip to content

Conversation

@edoakes
Copy link
Collaborator

@edoakes edoakes commented Nov 18, 2025

While writing the follow up to ban stack-allocated RayletClient instances, @dayshah proposed a simpler solution: make the counter a shared_ptr instead of capturing the entire class.

Signed-off-by: Edward Oakes <[email protected]>
Signed-off-by: Edward Oakes <[email protected]>
Signed-off-by: Edward Oakes <[email protected]>
@edoakes edoakes requested a review from a team as a code owner November 18, 2025 16:10
@edoakes edoakes added the go add ONLY when ready to merge, run all tests label Nov 18, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly refactors RayletClient to remove the dependency on std::enable_shared_from_this. By converting pins_in_flight_ to a std::shared_ptr<std::atomic<int64_t>>, you've effectively decoupled the lifetime of the counter from the RayletClient instance, allowing it to be safely used in asynchronous callbacks without requiring the client object to be managed by a shared_ptr. The implementation is clean and achieves the intended goal. I have one minor suggestion to improve the clarity of a lambda capture.

@edoakes
Copy link
Collaborator Author

edoakes commented Nov 18, 2025

@dayshah any clever C++ magic you have up your sleeve that could be used to ban people from unknowingly capturing this in a callback in the future?

Signed-off-by: Edward Oakes <[email protected]>
Signed-off-by: Edward Oakes <[email protected]>
@edoakes edoakes enabled auto-merge (squash) November 18, 2025 16:30
@github-actions github-actions bot disabled auto-merge November 18, 2025 16:30
@dayshah
Copy link
Contributor

dayshah commented Nov 18, 2025

@dayshah any clever C++ magic you have up your sleeve that could be used to ban people from unknowingly capturing this in a callback in the future?

eh, you could have a wrapper class to create the callbacks, and the constructor could not take pointers and references. But imo not worth it...

@edoakes edoakes merged commit 1f4a170 into ray-project:master Nov 18, 2025
5 of 6 checks passed
edoakes added a commit to edoakes/ray that referenced this pull request Nov 18, 2025
…_this` (ray-project#58744)

While writing the [follow
up](https://github.com/ray-project/ray/pull/58660/files#r2538761540) to
ban stack-allocated `RayletClient` instances, @dayshah proposed a
simpler solution: make the counter a shared_ptr instead of capturing the
entire class.

---------

Signed-off-by: Edward Oakes <[email protected]>
aslonnie pushed a commit that referenced this pull request Nov 18, 2025
Cherry picks two changes:

- #58660
- #58744

---------

Signed-off-by: dragongu <[email protected]>
Signed-off-by: Edward Oakes <[email protected]>
Co-authored-by: dragongu <[email protected]>
Co-authored-by: gulonglong <[email protected]>
Aydin-ab pushed a commit to Aydin-ab/ray-aydin that referenced this pull request Nov 19, 2025
…_this` (ray-project#58744)

While writing the [follow
up](https://github.com/ray-project/ray/pull/58660/files#r2538761540) to
ban stack-allocated `RayletClient` instances, @dayshah proposed a
simpler solution: make the counter a shared_ptr instead of capturing the
entire class.

---------

Signed-off-by: Edward Oakes <[email protected]>
Signed-off-by: Aydin Abiar <[email protected]>
400Ping pushed a commit to 400Ping/ray that referenced this pull request Nov 21, 2025
…_this` (ray-project#58744)

While writing the [follow
up](https://github.com/ray-project/ray/pull/58660/files#r2538761540) to
ban stack-allocated `RayletClient` instances, @dayshah proposed a
simpler solution: make the counter a shared_ptr instead of capturing the
entire class.

---------

Signed-off-by: Edward Oakes <[email protected]>
ykdojo pushed a commit to ykdojo/ray that referenced this pull request Nov 27, 2025
…_this` (ray-project#58744)

While writing the [follow
up](https://github.com/ray-project/ray/pull/58660/files#r2538761540) to
ban stack-allocated `RayletClient` instances, @dayshah proposed a
simpler solution: make the counter a shared_ptr instead of capturing the
entire class.

---------

Signed-off-by: Edward Oakes <[email protected]>
Signed-off-by: YK <[email protected]>
SheldonTsen pushed a commit to SheldonTsen/ray that referenced this pull request Dec 1, 2025
…_this` (ray-project#58744)

While writing the [follow
up](https://github.com/ray-project/ray/pull/58660/files#r2538761540) to
ban stack-allocated `RayletClient` instances, @dayshah proposed a
simpler solution: make the counter a shared_ptr instead of capturing the
entire class.

---------

Signed-off-by: Edward Oakes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go add ONLY when ready to merge, run all tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants