[core] Use shared_ptr for pins_in_flight_ instead of shared_from_this#58744
Merged
edoakes merged 5 commits intoray-project:masterfrom Nov 18, 2025
Merged
[core] Use shared_ptr for pins_in_flight_ instead of shared_from_this#58744edoakes merged 5 commits intoray-project:masterfrom
shared_ptr for pins_in_flight_ instead of shared_from_this#58744edoakes merged 5 commits intoray-project:masterfrom
Conversation
Contributor
There was a problem hiding this comment.
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.
Collaborator
Author
|
@dayshah any clever C++ magic you have up your sleeve that could be used to ban people from unknowingly capturing |
jjyao
approved these changes
Nov 18, 2025
Contributor
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
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 <ed.nmi.oakes@gmail.com>
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 <ed.nmi.oakes@gmail.com> Signed-off-by: Aydin Abiar <aydin@anyscale.com>
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 <ed.nmi.oakes@gmail.com>
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 <ed.nmi.oakes@gmail.com> Signed-off-by: YK <1811651+ykdojo@users.noreply.github.com>
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 <ed.nmi.oakes@gmail.com>
Future-Outlier
pushed a commit
to Future-Outlier/ray
that referenced
this pull request
Dec 7, 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 <ed.nmi.oakes@gmail.com> Signed-off-by: Future-Outlier <eric901201@gmail.com>
Aydin-ab
pushed a commit
to kunling-anyscale/ray
that referenced
this pull request
Dec 9, 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 <ed.nmi.oakes@gmail.com> Signed-off-by: Aydin Abiar <aydin@anyscale.com>
peterxcli
pushed a commit
to peterxcli/ray
that referenced
this pull request
Feb 25, 2026
…_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 <ed.nmi.oakes@gmail.com> Signed-off-by: peterxcli <peterxcli@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While writing the follow up to ban stack-allocated
RayletClientinstances, @dayshah proposed a simpler solution: make the counter a shared_ptr instead of capturing the entire class.