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

Change Connection traits to return futures #155

Closed
wants to merge 30 commits into from

Conversation

Ruben2424
Copy link
Contributor

Hi,
i experimented a bit with quinn 0.9 from #145 and GATs in the Quic Connection trait.
I changed the trait and implemented it for quinn. And i refactored the h3 implementation to use the new trait.
Of curse this is not finished yet. There are still some failing tests, I removed some code parts that need some refactoring and documentation is missing. But the examples mostly work.

The disadvantages of this:

  1. Breaks MSRV
  2. Changes the Traits
  3. Some other disadvantages I didn't think of?

Advantages:

  1. No Boxing required for the connection trait.
  2. Less Poll methods and because of that a simpler code base.
  3. Maybe some other advantages I didn't think of?

Open Questions:

  1. Do we need a driver for server like the client have one to manage the control and qpack streams?
  2. Should we change other traits to return futures as well?

I wanted to ask for your opinion before I continue to work in this. Is this the direction this project should go?

@Ruben2424
Copy link
Contributor Author

@seanmonstar @eagr what do you think?

@Ruben2424
Copy link
Contributor Author

Polling data from the control stream when accepting a new request like before didn\´t work well with the async methods. So I separated the accept logic so one task can manage the control stream while the other is accepting requests. The client is already doing a similar thing.
This fixed some tests. But there is still a lot of work to do.

@Ruben2424 Ruben2424 closed this May 31, 2024
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.

None yet

2 participants