Skip to content

feat: Implement ThreadAware for more more types#569

Open
AdomasBekeras wants to merge 6 commits into
mainfrom
u/abekeras/thread-aware-types
Open

feat: Implement ThreadAware for more more types#569
AdomasBekeras wants to merge 6 commits into
mainfrom
u/abekeras/thread-aware-types

Conversation

@AdomasBekeras

@AdomasBekeras AdomasBekeras commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Implement ThreadAware for std::net::SocketAddr

Copilot AI review requested due to automatic review settings July 13, 2026 10:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR expands thread_aware’s coverage of “inert / relocation-no-op” types by adding a feature-gated ThreadAware implementation for tokio::sync::mpsc::UnboundedSender<T> and by adding a no-op impl for std::net::SocketAddr, along with tests and documentation updates.

Changes:

  • Add ThreadAware no-op relocation support for std::net::SocketAddr (with unit test).
  • Introduce a new tokio feature that enables a ThreadAware impl for tokio::sync::mpsc::UnboundedSender<T> (plus tests).
  • Update crate docs/README and metadata (allowed_external_types, dependency wiring) to reflect the new tokio feature.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
crates/thread_aware/src/third_party/tokio.rs Adds the feature-gated ThreadAware impl for tokio::sync::mpsc::UnboundedSender<T> and validates behavior via tests.
crates/thread_aware/src/third_party/mod.rs Registers the new tokio third-party module behind cfg(any(test, feature = "tokio")) and updates module docs.
crates/thread_aware/src/lib.rs Documents the new tokio opt-in feature in the crate-level feature list.
crates/thread_aware/src/impls.rs Adds a no-op ThreadAware impl for SocketAddr and a unit test verifying relocation is a no-op.
crates/thread_aware/README.md Updates the generated README feature list to include tokio.
crates/thread_aware/Cargo.toml Adds the tokio feature, dependency/dev-dependency entries, and allowlists the external Tokio type for external-types checking.
Cargo.lock Records the new tokio dependency for thread_aware.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@AdomasBekeras AdomasBekeras changed the title Implement ThreadAware for more more types feat: Implement ThreadAware for more more types Jul 13, 2026
@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (f4ef129) to head (f35fde1).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #569   +/-   ##
=======================================
  Coverage   100.0%   100.0%           
=======================================
  Files         359      359           
  Lines       27800    27800           
=======================================
  Hits        27800    27800           
Flag Coverage Δ
linux 93.0% <ø> (-7.0%) ⬇️
linux-arm 93.0% <ø> (-7.0%) ⬇️
scheduled ?
windows 93.0% <ø> (-7.0%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI review requested due to automatic review settings July 13, 2026 10:26

@sandersaares sandersaares left a comment

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.

We need to define clear acceptance criteria in docs/DESIGN.md for why/when it is OK to add such out-of-box ThreadAware implementations. Today this feels like we are just randomly adding stuff here and I do not really see a rationale or a way to review whether something is OK to include or not in this way today. Certainly, including one random piece of Tokio feels weird (why not other pieces?).

@ralfbiedert can you help create these acceptance criteria so we can review this PR and other similar future PRs with some grounding?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.

@AdomasBekeras

Copy link
Copy Markdown
Contributor Author

We need to define clear acceptance criteria in docs/DESIGN.md for why/when it is OK to add such out-of-box ThreadAware implementations. Today this feels like we are just randomly adding stuff here and I do not really see a rationale or a way to review whether something is OK to include or not in this way today. Certainly, including one random piece of Tokio feels weird (why not other pieces?).

@ralfbiedert can you help create these acceptance criteria so we can review this PR and other similar future PRs with some grounding?

I'm adding it because Ralf asked to add it so I assume it's ok to add it here. I don't have a comment on the acceptance criteria but that's not really in the scope of this PR.

@ralfbiedert

Copy link
Copy Markdown
Collaborator

I'm adding it because Ralf asked to add it so I assume it's ok to add it here. I don't have a comment on the acceptance criteria but that's not really in the scope of this PR.

Sorry there was a misunderstanding. Adding ThreadAware for std items is generally fine, adding extra crates not so much. In fact, I'm a bit unhappy we have the existing ones already and we should consider removing them for 1.0 as they must live in the core crate and will be perma stable.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6503296c-52d4-4236-82bb-958903e3e519
Copilot AI review requested due to automatic review settings July 13, 2026 11:34
@AdomasBekeras

Copy link
Copy Markdown
Contributor Author

Removed the impl for the channel now

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@sandersaares sandersaares left a comment

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.

I'm adding it because Ralf asked to add it so I assume it's ok to add it here. I don't have a comment on the acceptance criteria but that's not really in the scope of this PR.

This, combined with Ralf's reply, highlights the importance of including acceptance criteria. Let's please add at least a statement into AGENTS.md or DESIGN.md or similar that we can only add implementations for std without special exception here. If you think this is an egregious violation of this PR's scope, I am OK if you just file a work item to add this line later.

Comment thread crates/thread_aware/src/impls.rs Outdated
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6503296c-52d4-4236-82bb-958903e3e519
Copilot AI review requested due to automatic review settings July 14, 2026 14:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings July 22, 2026 10:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@github-actions

Copy link
Copy Markdown

⚠️ Potential breaking changes detected

cargo semver-checks flagged the following on this PR. This is informational -- breaking changes between commits are expected; the major-version bump happens at release time, not on every PR.

bytesbuf

    Building bytesbuf v0.6.0 (current)
       Built [   4.160s] (current)
     Parsing bytesbuf v0.6.0 (current)
      Parsed [   0.005s] (current)
    Building bytesbuf v0.6.0 (baseline)
       Built [   4.123s] (baseline)
     Parsing bytesbuf v0.6.0 (baseline)
      Parsed [   0.004s] (baseline)
    Checking bytesbuf v0.6.0 -> v0.6.0 (no change; assume minor)
     Checked [   0.022s] 196 checks: 195 pass, 1 fail, 0 warn, 49 skip

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/inherent_method_missing.ron

Failed in:
  BytesView::get_num_le, previously in file /home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/bytesbuf-0.6.0/src/view_get.rs:232
  BytesView::get_num_be, previously in file /home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/bytesbuf-0.6.0/src/view_get.rs:319
  BytesView::get_num_ne, previously in file /home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/bytesbuf-0.6.0/src/view_get.rs:409
  BytesBuf::put_num_le, previously in file /home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/bytesbuf-0.6.0/src/buf_put.rs:211
  BytesBuf::put_num_be, previously in file /home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/bytesbuf-0.6.0/src/buf_put.rs:238
  BytesBuf::put_num_ne, previously in file /home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/bytesbuf-0.6.0/src/buf_put.rs:266

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [   9.905s] bytesbuf

fetch

    Building fetch v0.13.0 (current)
       Built [  34.341s] (current)
     Parsing fetch v0.13.0 (current)
      Parsed [   0.007s] (current)
    Building fetch v0.13.0 (baseline)
       Built [  34.872s] (baseline)
     Parsing fetch v0.13.0 (baseline)
      Parsed [   0.007s] (baseline)
    Checking fetch v0.13.0 -> v0.13.0 (no change; assume minor)
     Checked [   0.020s] 196 checks: 194 pass, 2 fail, 0 warn, 49 skip

--- failure method_parameter_count_changed: pub method parameter count changed ---

Description:
A publicly-visible method now takes a different number of parameters, not counting the receiver (self) parameter.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#fn-change-arity
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/method_parameter_count_changed.ron

Failed in:
  fetch::handlers::Metrics::layer now takes 0 parameters instead of 1, in /home/runner/work/oxidizer/oxidizer/crates/fetch/src/handlers/metrics.rs:235
  fetch::handlers::Logging::layer now takes 0 parameters instead of 2, in /home/runner/work/oxidizer/oxidizer/crates/fetch/src/handlers/logging.rs:43

--- failure method_requires_different_generic_type_params: method now requires a different number of generic type parameters ---

Description:
A method now requires a different number of generic type parameters than it used to. Uses of this method that supplied the previous number of generic types will be broken.
        ref: https://doc.rust-lang.org/reference/items/generics.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.46.0/src/lints/method_requires_different_generic_type_params.ron

Failed in:
  fetch::HttpClientBuilder::meter_provider takes 1 generic types instead of 0, in /home/runner/work/oxidizer/oxidizer/crates/fetch/src/client_builder.rs:254

     Summary semver requires new major version: 2 major and 0 minor checks failed
    Finished [  73.908s] fetch

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.

6 participants