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

fix master branch build. change web::block output type. #1957

Merged
merged 3 commits into from
Feb 6, 2021

Conversation

fakeshadow
Copy link
Contributor

@fakeshadow fakeshadow commented Feb 6, 2021

PR Type

Refactor

PR Checklist

  • Tests for the changes have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt.
  • (Team) Label with affected crates and semver status.

Overview

Change web::block to output any type the closure gives in Ok variant.(Send + 'static bound).

Simplify BlockingError type to only triggered by blocking threadpool dead.

Reduce size of actix_files::ChunkReadFile by 8 bytes.

Temoprary fix git build by pin dep version.

@fakeshadow fakeshadow added A-files project: actix-files A-http project: actix-http A-web project: actix-web B-semver-major breaking change requiring a major version bump labels Feb 6, 2021
@fakeshadow fakeshadow mentioned this pull request Feb 6, 2021
3 tasks
ChunkedReadFileState::Future(ref mut fut) => {
let (file, bytes) =
ready!(Pin::new(fut).poll(cx)).map_err(|_| BlockingError)??;
this.state = ChunkedReadFileState::File(Some(file));
Copy link
Contributor

@dunnock dunnock Feb 6, 2021

Choose a reason for hiding this comment

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

I wonder would it decrease number of polls spawning next chunk future right here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the structure of this future. The caller is responsible to actively poll an extra round when he get Some(Bytes) from the stream to go into the file branch again. Otherwise this stream would be stuck.
And I believe downstream API would be doing this. This is an internal stream chain that I think it's better to leave this way. Using Context::waker::wake_by_ref here certainlly is a good way to self wakeup but I'm afraid it would have a liitle bit more overhead than the downstream caller handling it(or result in more polling than needed as it does not aware this change)

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, makes sense in that case

Copy link
Contributor

@dunnock dunnock left a comment

Choose a reason for hiding this comment

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

LGTM, left a question if we can skip some polls in ChunkedFile Stream impl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-files project: actix-files A-http project: actix-http A-web project: actix-web B-semver-major breaking change requiring a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants