Upgrade quinn to 0.10 #58
Conversation
This commit attempts to upgrade quinn, but unfortunately many unit tests are failing. I'm not sure if the tests are valid given the updated quinn or if the updated code isn't compatible for some reason. The biggest thorn in this upgrade is that Quinn no longer implements Stream for some of its types, hence the manual pinning and looping done in some locations. I believe the way these are written is fine, since the select macros are going to drop the future in the situation of a shutdown anyways, so there's no worry about aborting an in-progress future and then losing state by asking for a new incoming connection.
|
Today I attempted to add two different signals to the incoming connection task: full shutdown vs shutdown incoming. This allowed me to have a loop that called Unfortunately, despite the immediate drop, the client still considers it a success and returns Ok. It feels messy, but I think the only way to get quinn to reject connections is to rebind the server to a new unknown address... but that doesn't actually stop the listening, it just moves it somewhere else. I can't see any way to actually disable incoming connections, and as such, I'm not sure fabruic's existing API can continue to work as-designed. |
FWIW this seems like a reasonable thing for us to have. |
After the last commit, I wasn't able to figure out how to get new connections to be rejected. I asked, and while there may be a better way to do this in the future, providing a new ServerConfig with the concurrent connections set to 0 blocks new connections from being formed. Thank you to both @djc and @Ralith for the help on this!
I've relaxed the clippy warnings on this crate to pedantic only. This led to a few warnings that are resolved by the small refactorings. This commit also sets the MSRV, which is currently being updated due to using std's pin!
When refactoring away from streams, I thought I could simplify the select_biased logic. I thought I could do this because the once the task is shut down, it should be able to be dropped. I'm not sure which part of this did not work as expected, but by properly looping and handling the 'complete' flow, everything works as intended. The other change that was needed, however, is to update Endpoint's close to also explicitly close incoming. Without this, the task is never shut down since calling close() doesn't abort the pending Accept future.
As much as I wanted to make select_biased work, I just couldn't. The example was hanging with the previous implementation, and my attempts at fixing it were fruitless without also breaking the unit tests. I decided to try to hand rolling the futures, and magically, everything worked. Because Accept isn't Unpin, it's a little ugly and requires a Box::pin currently. I don't love having that allocation there, but I couldn't figure out how to get the right access through pin_project, and I know this approach is 100% safe. It probably can be done safely without the allocation, however, as the only time that the accept field needs to be overwritten is after the future has been polled -- thus the Pin is not longer being used. I feel like this logic should have been able to be representable with select/select_biased, which might avoid the issue entirely, as pin! seemingly was able to deal Accept being Unpin, since it was being stored on the stack. However, I failed to get the logic to get both the example *and* the unit test working at the same time.
|
As per Discord, I'm merging without waiting for a review. I'm not going to release an update with this code until I've been able to run the BonsaiDb test suite against it -- which requires me to update a lot of dependencies now that I can update to the current rustls. The only thing I'm not happy with is that I ended up manually implementing futures when I felt like I should have been able to get select/select_biased to work correctly. |
This PR is an attempt at updating to quinn 0.10.
As of the initial push, there are three unit tests that are failing:
quic::endpoint::test::close_incoming: I cannot figure out how to get quinn to reject incoming connections, now thatIncomingBiStreamsisn't a thing anymore.quic::endpoint::test::close/quic::endpoint::test::close_and_reuse: time out due to not getting None from a closed server.Closes #59, #60, #61.