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

Quic and Slic fixes #2379

Merged
merged 7 commits into from
Dec 26, 2022
Merged

Quic and Slic fixes #2379

merged 7 commits into from
Dec 26, 2022

Conversation

bentoi
Copy link
Contributor

@bentoi bentoi commented Dec 22, 2022

This PR provides a number of fixes:

  • it fixes the Quic multiplexed stream to no longer dispose the Quic stream when reads and writes are closed. Doing this is opens a can of worms. We're back to the previous behavior where the Quic stream is disposed when Input/Output are completed.
  • it aligns the Slic ReadsClosed semantics with Quic, ReadsClosed is completed as soon as the last piece of data is returned from ReadAsync (instead of being completed when the data is singled consumed by the app with AdvanceTo)
  • it simplifies the stream ReadsClosed and WritesClosed tasks to no longer raise exceptions.
  • it updates some code in the icerpc protocol connection implementation to no longer dispose the dispatchCts or invocationCts from the unregister stream callback.

Protocol tests with Quic now work. It fixes #2287 but it doesn't fix #2300 or #2348.

@bernardnormier
Copy link
Member

Can you open a proposal that describes what you've implemented here?

Keeping an async-disposable multiplexed stream where DisposeAsync:

  • does nothing for Quic
  • does not complete Input/Output with Slic

is not a correct API in my view.

@bentoi
Copy link
Contributor Author

bentoi commented Dec 23, 2022

Can you open a proposal that describes what you've implemented here?

Opened #2381

@bentoi bentoi requested a review from pepone December 23, 2022 16:52
Copy link
Member

@pepone pepone left a comment

Choose a reason for hiding this comment

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

Looks good, there are still a few references to DiposeAsync in SlicStream comments. There are also a few background tasks that do not handle non-IceRpcExceptions and will end up as UTE, I guess we can fix this in a separate PR for #2351

@bentoi bentoi merged commit 93ace7e into icerpc:main Dec 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants