Skip to content

Dist_graph_create*: Finalize the created communicators #970

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.

Sign up for GitHub

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

Open
eschnett wants to merge 4 commits into
base: master
Choose a base branch
from
+33 −1
Open

Dist_graph_create*: Finalize the created communicators #970

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8b4bbbc
Dist_graph_create*: Finalize the created communicators
eschnett Sep 8, 2026
3970ac8
Merge branch 'master' into claude/correctness-issue-7-distgraph-free
eschnett Sep 9, 2026
9ef40eb
Merge branch 'master' into claude/correctness-issue-7-distgraph-free
eschnett Sep 9, 2026
776b6c1
Merge branch 'master' into claude/correctness-issue-7-distgraph-free
eschnett Sep 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/topology.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,8 @@ const WEIGHTS_EMPTY = WeightsEmpty()

Create a new communicator from a given directed graph topology, described by local incoming and outgoing edges on an existing communicator.

[`MPI.free`](@ref) may be called on the returned communicator once it is no longer needed; otherwise it is freed at finalization.

# Arguments
- `comm::Comm`: The communicator on which the distributed graph topology should be induced.
- `sources::Vector{Cint}`: The local, incoming edges on the rank of the calling process.
Expand Down Expand Up @@ -229,6 +231,7 @@ function Dist_graph_create_adjacent(comm::Comm, sources::Vector{Cint}, destinati
length(sources), sources, source_weights,
length(destinations), destinations, destination_weights,
Info(infokws...), reorder, graph_comm)
graph_comm != COMM_NULL && finalizer(free, graph_comm)

@vchuravy vchuravy Sep 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When does graph_comm == COMM_NULL happen? If that is a case we need to worry about I would rather have the check in free since the docs now tells the user to call free.

@eschnett eschnett Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The standard doesn't mention that it could be COMM_NULL, I think it cannot. This code just follows the usual pattern, and that pattern isn't necessary here.

Should be we be defensive? This way we keep the check here and also add it inside the finalizer.

return graph_comm
end

Expand All @@ -237,6 +240,8 @@ end

Create a new communicator from a given directed graph topology, described by incoming and outgoing edges on an existing communicator.

[`MPI.free`](@ref) may be called on the returned communicator once it is no longer needed; otherwise it is freed at finalization.

# Arguments
- `comm::Comm`: The communicator on which the distributed graph topology should be induced.
- `sources::Vector{Cint}`: An array with the ranks for which this call will specify outgoing edges.
Expand Down Expand Up @@ -266,7 +271,7 @@ function Dist_graph_create(comm::Comm, sources::Vector{Cint}, degrees::Vector{Ci
# MPI_Info info, int reorder, MPI_Comm *comm_dist_graph)
API.MPI_Dist_graph_create(comm, length(sources), sources, degrees, destinations, weights,
Info(infokws...), reorder, graph_comm)

graph_comm != COMM_NULL && finalizer(free, graph_comm)
return graph_comm
end

Expand Down
27 changes: 27 additions & 0 deletions test/test_neighbor_comm.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,32 @@ let
@test srcw2 == srcw3 == Cint[prev_rank + comm_size]
end

# Both `Dist_graph_create*` must attach a finalizer, like every other
# communicator constructor, so that the communicator is not leaked.
let
graph_comm = ring_graph(; weighted=false)
@test graph_comm != MPI.COMM_NULL
finalize(graph_comm)
@test graph_comm == MPI.COMM_NULL
end

let
sources = Cint[prev_rank]
destinations = Cint[next_rank]
graph_comm = MPI.Dist_graph_create_adjacent(comm, sources, destinations)
@test graph_comm != MPI.COMM_NULL
finalize(graph_comm)
@test graph_comm == MPI.COMM_NULL
end

# An explicit `free` must compose with the finalizer rather than double-free.
let
graph_comm = ring_graph(; weighted=false)
MPI.free(graph_comm)
@test graph_comm == MPI.COMM_NULL
finalize(graph_comm)
@test graph_comm == MPI.COMM_NULL
end

MPI.Finalize()
@test MPI.Finalized()
Loading