Skip to content

Conversation

@Ralith
Copy link
Collaborator

@Ralith Ralith commented Jan 7, 2020

We're already reliant on tokio's thread locals for UDP socket and timer registration, and Handle::enter provides adequate mechanism to route spawns to a particular runtime if absolutely needed, so passing driver tasks to the user explicitly wasn't buying us much considering how big a footgun it is.

@Ralith Ralith force-pushed the spawn-drivers branch 3 times, most recently from c4a2a02 to 87b2e30 Compare January 7, 2020 06:49
@Ralith
Copy link
Collaborator Author

Ralith commented Jan 7, 2020

I'm fairly confident that this is the direction I'd like to go. Just need to update more tests and, at least ideally, find a way to test that the implicitly spawned tasks get cleaned up properly.

@djc
Copy link
Member

djc commented Jan 7, 2020

Looks okay to me. I'll give it a full review once it's complete.

@Ralith Ralith force-pushed the spawn-drivers branch 2 times, most recently from 6606da1 to 8cd63b6 Compare January 10, 2020 06:17
@Ralith
Copy link
Collaborator Author

Ralith commented Jan 10, 2020

Rebased onto #589 and #590, plus a couple additional bugfixes. I think this is ready now, modulo the dependency PRs being merged.

@Ralith Ralith marked this pull request as ready for review January 10, 2020 06:19
@Ralith
Copy link
Collaborator Author

Ralith commented Jan 10, 2020

Er, right, still need to port h3.

@Ralith Ralith force-pushed the spawn-drivers branch 2 times, most recently from ed71f75 to 92eb642 Compare January 10, 2020 08:20
@github-actions
Copy link

Pull Request Test Coverage Report for Build 612d7479973aa2250e24254c65764344853adc0b-PR-586

  • 1 of 19 (5.26%) changed or added relevant lines in 4 files are covered.
  • 19 unchanged lines in 6 files lost coverage.
  • Overall coverage decreased (-0.1%) to 70.536%

Changes Missing Coverage Covered Lines Changed/Added Lines %
quinn-h3/src/connection.rs 0 3 0.0%
quinn-h3/src/server.rs 0 5 0.0%
quinn-h3/src/client.rs 0 10 0.0%
Files with Coverage Reduction New Missed Lines %
quinn-proto/src/packet.rs 1 86.44%
quinn-h3/src/client.rs 2 0.0%
quinn-proto/src/shared.rs 3 47.67%
../../../../../usr/share/rust/.cargo/registry/src/github.com-1ecc6299db9ec823/tracing-futures-0.2.0/src/lib.rs 3 66.67%
quinn-h3/src/server.rs 4 0.0%
../../../../../usr/share/rust/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-channel-0.3.1/src/mpsc/mod.rs 6 78.34%
Totals Coverage Status
Change from base Build 29dfcab51f5000c1f553fb5b8a97a9a0758c9b1b: -0.1%
Covered Lines: 9911
Relevant Lines: 14051

💛 - Coveralls

@Ralith Ralith force-pushed the spawn-drivers branch 2 times, most recently from f58376c to f907b53 Compare January 10, 2020 08:36
@Ralith
Copy link
Collaborator Author

Ralith commented Jan 10, 2020

Okay, now we're good. This is still missing checks that we're not leaking tasks, but I'm not sure if there's a good way to do that with tokio 0.2 yet.

@Ralith Ralith requested a review from stammw January 10, 2020 08:43
@Ralith
Copy link
Collaborator Author

Ralith commented Jan 10, 2020

On consideration, I think I can refactor the whole connection setup path to be a lot more straightforward; going to try to get that in before merging.

@Ralith Ralith force-pushed the spawn-drivers branch 2 times, most recently from ac70cf4 to 6210604 Compare January 11, 2020 02:09
@Ralith
Copy link
Collaborator Author

Ralith commented Jan 11, 2020

Okay, I think this is ready.

stammw
stammw previously approved these changes Jan 12, 2020
@Ralith
Copy link
Collaborator Author

Ralith commented Jan 12, 2020

Rebased.

@djc
Copy link
Member

djc commented Jan 13, 2020

This needs one more rebase...

@djc djc merged commit 47ff6b5 into master Jan 13, 2020
@delete-merged-branch delete-merged-branch bot deleted the spawn-drivers branch January 13, 2020 08:54
@stammw
Copy link
Contributor

stammw commented Jan 13, 2020

🎉

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.

4 participants