Skip to content

Conversation

@Tratcher
Copy link
Member

@Tratcher Tratcher commented Aug 30, 2022

Marking some unfinished quic options as preview so we can continue working on them in .NET 8.

Description

We don't expect to finish #32034 this release so I'm marking the affected APIs as preview. These are not commonly used, the impact should be minimal.

Customer Impact

Customers using these options will get a suppressible warning.

Regression?

  • Yes
  • No

The entire quic feature was marked as preview in 6.0. That was removed early in 7.0, but we're going to mark some specific options as preview now.

Risk

  • High
  • Medium
  • Low

Only introduces warnings.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@Tratcher Tratcher added this to the 7.0-rc2 milestone Aug 30, 2022
@Tratcher Tratcher self-assigned this Aug 30, 2022
@ghost ghost added the area-runtime label Aug 30, 2022
@Pilchie
Copy link
Member

Pilchie commented Aug 30, 2022

This has essentially been public for all of .NET 7, so I feel like we should treat this as an API change and give Tactics a heads up. Are we worried about there being people out there using it?

@Tratcher
Copy link
Member Author

It was preview in 6 and earlier in .NET 7. These are not commonly used APIs, and at most people using them will get a suppressible warning.

@Pilchie Pilchie added the Servicing-consider Shiproom approval is required for the issue label Aug 31, 2022
@ghost
Copy link

ghost commented Aug 31, 2022

Hi @Tratcher. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

Do we also want to add [RequiresPreviewFeatures] to DefaultStreamErrorCode and DefaultCloseErrorCode? Those seem like things that don't really belong in the transport layer and we might want to remove later. @JamesNK

It might also make sense to forward the tactics email to the API reviews+reviewers DLs to inform anyone else who might have opinions about this PR.

@Tratcher Tratcher added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Aug 31, 2022
@JamesNK
Copy link
Member

JamesNK commented Sep 1, 2022

DefaultStreamErrorCode and DefaultCloseErrorCode should be here.

DefaultCloseErrorCode = if someone disposes a stream or connection without explicitly closing it then an abort code needs to be sent to the client. This defines that value.

DefaultStreamErrorCode = if there is a stream error (I'm not exactly sure what the cause could be) then an error code is reported. This defines that value.

These values are configured when a QUIC connection is started.

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

Who is expected to read or set DefaultStream/CloseErrorCode @JamesNK? What values would they change these to and why?

If it's for use by Kestrel.Core, I'd still just mark it experiential for now. I could see us exclusively setting the status code with abort or some other features before disposing the connection or stream in the future.

That said, everything marked experimental seems reasonable to me. I could see us keeping some of these like MaxReadBufferSize that have analogs in SocketTransportOptions, but we can unmark it later.

@adityamandaleeka adityamandaleeka merged commit 5e8f997 into dotnet:release/7.0 Sep 1, 2022
@Tratcher Tratcher deleted the tratcher/noquicoptions branch September 1, 2022 18:49
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Servicing-approved Shiproom has approved the issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants