Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Write tests for methods added during improving socket output performance #1508

Closed
pakrym opened this issue Mar 18, 2017 · 5 comments
Closed
Assignees

Comments

@pakrym
Copy link
Contributor

pakrym commented Mar 18, 2017

In this PR: #1504
/cc @davidfowl

@muratg muratg added this to the 2.0.0 milestone Mar 20, 2017
@muratg muratg assigned pakrym and natemcmaster and unassigned pakrym Mar 20, 2017
@muratg
Copy link
Contributor

muratg commented Mar 20, 2017

Two tests:

  • Write numeric
  • Write ASCII

Also investigate if this would cause issues on writing unicode.

@natemcmaster
Copy link
Contributor

I'm looking at the unicode issue. @davidfowl you seemed to thing the Unicode + WriteAscii method is okay. What was the justification?

Before #1504, we used Encoding.ASCII.GetBytes which does a range change on data and replaces out-of-range characters with '?'. The new implementation doesn't range check.
Comparison:

Before: WriteAscii("ñ٢⛄⛵") => "????"
After:  WriteAscii("ñ٢⛄⛵") => "ñbõÄ"

The concern here is that it's possible this method will transform non-ascii into ascii, e.g. ٢ => b.
cc @benaadams

@benaadams
Copy link
Contributor

@natemcmaster as per current Kestrel.

They are response headers and ValidateHeaderCharacters is run on them when they are added to the collections (so they throw an exception in user code when added if not ascii) so there is no need to check them again on output.

@davidfowl
Copy link
Member

@natemcmaster if you want to feel better about the method, you can rename it to WriteAsciiNoValidation

@natemcmaster
Copy link
Contributor

Thanks for clarifying.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants