Skip to content

Commit 6c266bf

Browse files
pzhan9meta-codesync[bot]
authored andcommitted
Update the bound field within PortHandle.bind_default method (#1439)
Summary: Pull Request resolved: #1439 Currently, `PortHandle.bind_default` does not save `port_index` in the handle's `bound` field after it binds the port. This diff changes that. In this way, we can know whether the handle object is bound, and bound to which port, by using `PortHandle::location` method. This change is a behavior change to another method, `PortHandle::bind`. Previously, `bind` can still mutate the `bound` field aster `bind_default` was called. After this diff, it cannot. The new behavior is documented in the new test, `test_bind_port_handle_to_default`. Reviewed By: mariusae Differential Revision: D83934172 fbshipit-source-id: e761c1f697dc26b219a01f45046a1421ca0bd065
1 parent 1731e62 commit 6c266bf

File tree

1 file changed

+47
-0
lines changed

1 file changed

+47
-0
lines changed

hyperactor/src/mailbox.rs

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1644,6 +1644,16 @@ impl<M: RemoteMessage> PortHandle<M> {
16441644
/// This is used by [`actor::Binder`] implementations to bind actor refs.
16451645
/// This is not intended for general use.
16461646
pub(crate) fn bind_actor_port(&self) {
1647+
let port_id = self.mailbox.actor_id().port_id(M::port());
1648+
self.bound
1649+
.set(port_id)
1650+
.map_err(|p| {
1651+
format!(
1652+
"could not bind port handle {} as {p}: already bound",
1653+
self.port_index
1654+
)
1655+
})
1656+
.unwrap();
16471657
self.mailbox.bind_to_actor_port(self);
16481658
}
16491659
}
@@ -3763,4 +3773,41 @@ mod tests {
37633773
.unwrap();
37643774
assert_eq!(rx.recv().await.unwrap(), (1, "hello".to_string()));
37653775
}
3776+
3777+
#[test]
3778+
#[should_panic(expected = "already bound")]
3779+
fn test_bind_port_handle_to_actor_port_twice() {
3780+
let mbox = Mailbox::new_detached(id!(test[0].test));
3781+
let (handle, _rx) = mbox.open_port::<String>();
3782+
handle.bind_actor_port();
3783+
handle.bind_actor_port();
3784+
}
3785+
3786+
#[test]
3787+
fn test_bind_port_handle_to_actor_port() {
3788+
let mbox = Mailbox::new_detached(id!(test[0].test));
3789+
let default_port = mbox.actor_id().port_id(String::port());
3790+
let (handle, _rx) = mbox.open_port::<String>();
3791+
// Handle's port index is allocated by mailbox, not the actor port.
3792+
assert_ne!(default_port.index(), handle.port_index);
3793+
// Bind the handle to the actor port.
3794+
handle.bind_actor_port();
3795+
assert_matches!(handle.location(), PortLocation::Bound(port) if port == default_port);
3796+
// bind() can still be used, just it will not change handle's state.
3797+
handle.bind();
3798+
handle.bind();
3799+
assert_matches!(handle.location(), PortLocation::Bound(port) if port == default_port);
3800+
}
3801+
3802+
#[test]
3803+
#[should_panic(expected = "already bound")]
3804+
fn test_bind_port_handle_to_actor_port_when_already_bound() {
3805+
let mbox = Mailbox::new_detached(id!(test[0].test));
3806+
let (handle, _rx) = mbox.open_port::<String>();
3807+
// Bound handle to the port allocated by mailbox.
3808+
handle.bind();
3809+
assert_matches!(handle.location(), PortLocation::Bound(port) if port.index() == handle.port_index);
3810+
// Since handle is already bound, call bind_to() on it will cause panic.
3811+
handle.bind_actor_port();
3812+
}
37663813
}

0 commit comments

Comments
 (0)