Skip to content

refactor(core): move the outbound remote adapter operations into core behind remote-adapter feature - #824

Open
LiamCarPer wants to merge 1 commit into
Totodore:mainfrom
LiamCarPer:refactor/move-outbound-ops-to-core
Open

LiamCarPer wants to merge 1 commit into
Totodore:mainfrom
LiamCarPer:refactor/move-outbound-ops-to-core

Conversation

@LiamCarPer

Copy link
Copy Markdown
Contributor

Part of #727, closes #804.

Motivation

The outbound half of the CoreAdapter impl was still duplicated in the three adapters. broadcast, disconnect_socket, rooms, add_sockets, del_sockets and fetch_sockets were ~95% identical (~450 lines total), and the get_res server count snippet was copied six times (get_res x3 + broadcast_with_ack x3), which is exactly where the #782 bug came from.

Solution

  • New RemoteOperations trait in socketioxide-core behind remote-adapter, with default implementations of the six operations. Each adapter only implements local(), uid(), server_count(), send_req() and get_res().
  • Generic InitRes<E> in core, replacing the three identical copies (only the error type was different).
  • get_res and broadcast_with_ack now read the server count from RemoteOperations::server_count, so that logic lives in one place.

broadcast_with_ack stays adapter local for now. It needs associated types for the response stream plus its decoder, and it was just changed by the ack timeout (#822) and response handler leak (#802) fixes, so i would rather do it separately if you think it is worth it.

One behavior change: postgres and mongodb now target opts.server_id for broadcast and disconnect_socket (like redis already did) instead of sending the request to every server. It only affects the case where we act on a specific remote socket, where only the target server can have a match anyway.

Tests

No new tests: the existing fixture tests of the three adapters already cover every moved operation (broadcast, broadcast with ack, rooms, add/del sockets, fetch sockets, disconnect) and they pass with the shared implementations.

Moves the duplicated outbound CoreAdapter operations from the redis,
postgres and mongodb adapters into socketioxide-core behind the existing
remote-adapter feature flag.

- RemoteOperations trait with default implementations of broadcast,
  disconnect_socket, rooms, add_sockets, del_sockets and fetch_sockets.
  Adapters only provide local(), uid(), server_count(), send_req() and
  get_res().
- Generic InitRes<E> replacing the three identical copies.
- The response count of get_res is now read from RemoteOperations, so the
  server count logic lives in one place instead of six.

broadcast_with_ack stays adapter-local for now: it needs associated types
for the response stream and its decoder, and it was recently changed by
the ack timeout and response handler leak fixes.

One behavior change: postgres and mongodb now target opts.server_id for
broadcast and disconnect_socket like redis already did, instead of sending
the request to every server.

Closes Totodore#804
@codspeed

codspeed Bot commented Nov 2, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

1 warning

⚠️ 4 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

✅ 87 untouched benchmarks


Comparing LiamCarPer:refactor/move-outbound-ops-to-core (90ea093) with main (d03ed5c)

Open in CodSpeed

@LiamCarPer

Copy link
Copy Markdown
Contributor Author

Hey, the only red check is udeps and it looks unrelated to this branch. It fails while compiling mongodb 3.5.2:

error[E0391]: cycle detected when computing when `error::Error` needs async drop
  --> mongodb-3.5.2/src/error.rs:69:1

The udeps job uses the floating nightly toolchain, and main fails the exact same way right now (d03ed5c and 515aa26 both have udeps red). So it looks like a nightly regression on the mongodb side, not something from this PR.

Everything else is green: test, adapter, feature_set, clippy, msrv, format and the socket_io/engine_io suites.

If you want i can open a small separate PR to pin the nightly used by the udeps job (or skip the mongodb crate there) until nightly is fixed.

@LiamCarPer LiamCarPer closed this Nov 9, 2026
@LiamCarPer LiamCarPer reopened this Nov 9, 2026
@LiamCarPer LiamCarPer closed this Nov 9, 2026
@LiamCarPer LiamCarPer reopened this Nov 9, 2026
@LiamCarPer

Copy link
Copy Markdown
Contributor Author

Hey, just a small ping on this one when you have a moment.

CI is all green now: the earlier udeps failure was the nightly + mongodb issue that also hit main (it passes on the latest run), and the e2e failure was the socket_io (v5-msgpack) job connecting before the servers were up, which passed on the rerun. So nothing is blocked from my side, just waiting for a review.

This branch has not been deployed

No deployments
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.

Move the outbound remote adapter operations into core behind the remote-adapter feature flag

1 participant