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

[Kestrel] Support for custom decoder in headers #17400

Closed
analogrelay opened this issue Nov 25, 2019 · 5 comments
Closed

[Kestrel] Support for custom decoder in headers #17400

analogrelay opened this issue Nov 25, 2019 · 5 comments
Assignees
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-kestrel feature-yarp This issue is related to work on yarp

Comments

@analogrelay
Copy link
Contributor

Follow up from #17399

In 5.0 we want to provide a hook to use a custom decoder for headers. A System.Text.Encoding.Encoder is probably not quite sufficient since the codec may want to see the header name in order to make decisions. A strawman API would be something like this:

string Decode(string headerName, ReadOnlySpan<byte> headerBytes);
@stephentoub
Copy link
Member

stephentoub commented May 26, 2020

A strawman API would be something like this

Would it make sense to be more like:

Encoding? GetEncodingForHeader(string headerName);

?

I'm thinking that would enable:

  • more flexibility in the future, given the various APIs on Encoding
  • the ability to cache an Encoding to use for a given header, which could in turn enable caching opportunities for a given input sequence
  • potential devirtualization opportunities

Just a thought. Not sure if this has progressed since the original strawman.

@BrennanConroy BrennanConroy added Needs: Design This issue requires design work before implementating. feature-yarp This issue is related to work on yarp labels Jun 1, 2020
@BrennanConroy BrennanConroy assigned halter73 and unassigned shirhatti Jun 1, 2020
@halter73
Copy link
Member

halter73 commented Jun 4, 2020

Would it make sense to be more like:

Encoding? GetEncodingForHeader(string headerName);

I think that does make more sense. I assume a null return value tells the server or client to use its default behavior. Do you think an interface for this belongs in System.Net.Http @stephentoub?

@stephentoub
Copy link
Member

stephentoub commented Jun 4, 2020

Why would an interface be needed? It could just be a delegate?

(But, yes, is there's a good reason for this to be an interface, we should have one rather than two. I just prefer a delegate for a single operation as a) it's faster, b) it's easier for a caller to supply, and c) it doesn't require introducing a new type.)

@stephentoub
Copy link
Member

I assume a null return value tells the server or client to use its default behavior.

That's my suggestion, as would not providing a delegate/interface at all.

@halter73
Copy link
Member

halter73 commented Jun 5, 2020

Why would an interface be needed? It could just be a delegate?

It's not, and it could be. I was thinking about having custom decoders be registered via DI, but I understand this doesn't make sense for System.Net.

@halter73 halter73 changed the title [Kestrel] Support for custom decoder in headers and URL [Kestrel] Support for custom decoder in headers Jun 29, 2020
@halter73 halter73 removed the Needs: Design This issue requires design work before implementating. label Jun 29, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Jul 29, 2020
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-kestrel feature-yarp This issue is related to work on yarp
Projects
None yet
Development

No branches or pull requests

7 participants