Skip to content

Update to std::future #389

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

Merged
merged 7 commits into from
Aug 17, 2019
Merged

Update to std::future #389

merged 7 commits into from
Aug 17, 2019

Conversation

95th
Copy link
Contributor

@95th 95th commented Jul 27, 2019

Closes #385

@95th
Copy link
Contributor Author

95th commented Jul 28, 2019

For some reason, CI is using old version of Rust Nightly. Multiple lifetime issue has been resolved a month now. @seanmonstar How do I make it use latest Nightly?

@seanmonstar
Copy link
Member

I'm on vacation for a couple weeks with no computer and spotty internet. If no one else looks while I'm gone, I'll take a look when I get back.

Ping @carllerche @sgrif

@carllerche
Copy link
Collaborator

And @sgrif is sick. I def want to pull in one of the seans...

@95th
Copy link
Contributor Author

95th commented Aug 3, 2019

@carllerche Okay.. Meanwhile I'll finish up what's remaining. Current status:

Works:

  • Code builds
  • Internal h2 crate tests pass
  • h2spec passes
  • h2-test passes
  • doc tests pass
  • h2-fuzz compiles

In progress:

  • A few h2-tests fail.
    Failing mostly in [assert_client_handshake]
  • doc tests needs to be updated - I am thinking of doing it in a separate PR.
  • h2-fuzz - needs to updated - will update it after fixing the h2-tests
  • h2-fuzz needs to be tested - honggfuzz don't work on WSL and Travis is acting up. So can't really test.

cc @LucioFranco

@95th 95th marked this pull request as ready for review August 3, 2019 11:46
@95th
Copy link
Contributor Author

95th commented Aug 5, 2019

Since integrated CI doesnt use correct rust version, here is a build run against my fork:

https://travis-ci.org/95th/h2/builds/567710136

@95th
Copy link
Contributor Author

95th commented Aug 5, 2019

@carllerche
Most of the failing tests have similar errors:

expected SETTINGS; actual=Headers { stream_id: StreamId(1), flags: (0x5: END_HEADERS | END_STREAM) }
expected SETTINGS; actual=Headers { stream_id: StreamId(1), flags: (0x4: END_HEADERS) }

The error comes in assert_client_handshake when trying to assert settings ACK.

Any ideas?

cc @LucioFranco @sgrif

95th referenced this pull request in sgrif/h2 Aug 10, 2019
@sgrif
Copy link

sgrif commented Aug 14, 2019

Is there any way for you to break this up into smaller pieces? As I've been working through this my plan was to start with just h2-support and the tests, since those can be changed without touching the main crate, while the inverse is not true. 6k lines changed is a lot to review at once

@95th
Copy link
Contributor Author

95th commented Aug 15, 2019

Looks like my own PR rust-lang/rust#63514 has come back to bite me

@95th
Copy link
Contributor Author

95th commented Aug 15, 2019

@sgrif I rewrote commit history where each commit updates one area. Is that sufficient or do you want me to open separate PRs? BTW I have already updated h2-support, h2-tests, h2-fuzz also.

@95th 95th changed the title [WIP] Update to std::future Update to std::future Aug 15, 2019
Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

This is an awesome amount of work, thank you! Left some comments inline.

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

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

Excellent work!

@seanmonstar seanmonstar merged commit b039ef2 into hyperium:master Aug 17, 2019
@LucioFranco
Copy link
Member

@95th awesome job! 🎉

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.

Update to std::future
5 participants