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

Trying to improve the documentation for App::data() #1736

Closed
wants to merge 9 commits into from

Conversation

lfrancke
Copy link

@lfrancke lfrancke commented Oct 16, 2020

PR Type

Other (Documentation only)

PR Checklist

Check your PR fulfills the following:

  • Documentation comments have been added / updated.

Overview

This is related to the discussion in #1454 which aims to eventually change the semantics of data and/or app_data. In the meantime I found the documentation very confusing and in this PR I'm trying to clarify what's going on.

That said: I still don't fully understand it so I set it as a draft PR and am looking for feedback on what might be changed.

My main open question is why data wraps in an Arc at all.
I'm new to Rust so I might be missing very obvious things but by my understanding Actix runs the application factory closure once per thread it instantiates so the data does not need to be Send because it's instantiated in the target thread and it doesn't need to be Sync (by default) either, no?

If anyone could help clarify I'd try to adopt the documentation.

@JohnTitor
Copy link
Member

Oh, nightly encounters an ICE, I believe that's rust-lang/rust#77993.

src/app.rs Show resolved Hide resolved
@robjtede robjtede added A-web project: actix-web B-semver-norelease change that does not require a release labels Oct 16, 2020
@fakeshadow
Copy link
Contributor

extract Data in handler would require a Clone. That's the main reason it's a smart pointer so what inside the Data can be a not cloneable type. Arc is a reasonable choice as the cost is not big in single thread(almost no contetion) and it can be moved to other threads easily if you ever want to in your handler(like pass it to threadpool or a future moved to other threads).
It all comes down to let user pass a cloneable type to Data or the framework do it for them with a low cost wrapper.

@lfrancke
Copy link
Author

Thank you @fakeshadow, that is very helpful. I never looked at it that way. I'll try to adjust the docs.

Also thanks @JohnTitor I was a bit surprised that my docs-only change causes a failure.

@lfrancke lfrancke marked this pull request as ready for review October 27, 2020 18:29
src/app.rs Show resolved Hide resolved
@robjtede robjtede requested a review from a team January 5, 2021 12:49
Comment on lines +80 to +81
/// This means that only the first invocation of this function per type will have an effect,
/// all later ones will be ignored.
Copy link
Member

@robjtede robjtede Jan 11, 2021

Choose a reason for hiding this comment

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

Is this true, or is it the last invocation is used?

Copy link
Author

Choose a reason for hiding this comment

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

I did look at the code back then and I thought this is what it does but I can take another look.

Copy link
Author

Choose a reason for hiding this comment

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

I have to be honest: I don't understand (anymore) how this is used. If one of you has any insights I'll happily add it otherwise I'd just remove that sentence.

It would be good to document the behavior though.

Copy link
Member

Choose a reason for hiding this comment

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

You know what you're right, and that's surprising behavior actually because .app_data works differently; a bug, even.

Copy link
Author

Choose a reason for hiding this comment

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

Out of curiosity (because I couldn't trace it anymore but I know I did in the past): Can you point me at the location where this happens?

src/data.rs Outdated Show resolved Hide resolved
@lfrancke lfrancke closed this May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-web project: actix-web B-semver-norelease change that does not require a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants