Skip to content
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.

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

Avoid Double Arc for Arc<dyn> with T: ?Sized #1710

Closed
PeterUlb opened this issue Sep 25, 2020 · 1 comment · Fixed by #1712
Closed

Avoid Double Arc for Arc<dyn> with T: ?Sized #1710

PeterUlb opened this issue Sep 25, 2020 · 1 comment · Fixed by #1712
Labels
A-web project: actix-web C-improvement Category: an improvement to existing functionality

Comments

@PeterUlb
Copy link
Contributor

Expected Behavior

It should be possible to create web::Data from an Arc<dyn Xyz>.

Current Behavior

    // trait T{}, struct A{}, impl T for A{}
    // This works as of now
    let test: Arc<Box<dyn T>> = Arc::new(Box::new(A {}));
    let data = web::Data::from(test);
    // This should also work (see above with Sized)
    let test2: Arc<dyn T> = Arc::new(A {});
    let data2 = web::Data::from(test2);

=> the size for values of type dyn T cannot be known at compilation time

Possible Solution

Add Data<T: ?Sized>
Extensions.insert uses TypeId::of::<T>, where in this case T is Data<T: ?Sized>.
To extract we can use extensions.get::<Data<dyn T>>()

Steps to Reproduce (for bugs)

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7f31f526ea15a6c03ae768a900d822d0

Compare <T: ?Sized> (new version) with <T> (current version)

Context

I want to dynamically pass a Trait object to actix. This allows me to swap e.g. Service implementations with mock objects for unit tests.

Your Environment

  • Rust Version (I.e, output of rustc -V): rustc 1.46.0 (04488afe3 2020-08-24)
  • Actix Web Version: 3.0

Question is: Would it break anything else when adding ?Sized. Are there places relying on a known size? Quick glance shows that even the Arc is additionally Boxed in the Extensions HashMap.

@robjtede robjtede added C-improvement Category: an improvement to existing functionality A-web project: actix-web labels Sep 26, 2020
@robjtede
Copy link
Member

robjtede commented Sep 26, 2020

I did some testing and this idea seems to have merit. I think it would be okay as long as it is emphasized in the docs that only one (of each T) explicit dyn T is possible. Would welcome a PR to ascertain any wider impact.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-web project: actix-web C-improvement Category: an improvement to existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants