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

Prototype: unescapse path #269

Closed
wants to merge 1 commit into from
Closed

Prototype: unescapse path #269

wants to merge 1 commit into from

Conversation

troydai
Copy link
Contributor

@troydai troydai commented Oct 20, 2015

Issue #124

To unescape the path while read the first line to the request.

Note:

  • Implementation of the decoding is in HttpAbstraction. It is currently being reviewed: Url Path Decoder HttpAbstractions#448.
  • A new method is added to MemoryPoolIterator2 to return the raw char[] instead of a string as a result the decoding is able to be operated in place to avoid additional allocation.
  • The change will not increase additional memory allocation.

Follow up:

  • Add test cases.
  • Performance review afterwards.

/cc @halter73 @Tratcher @davidfowl @lodejard @muratg

@@ -623,6 +626,11 @@ private bool TakeStartLine(SocketInput input)
return false;
}

char[] requestUriChars;
var rawPathLength = beginPath.GetCharArray(endPath, out requestUriChars);
var decodedLength = UrlPathDecoder.DecodeInPlace(requestUriChars, rawPathLength);
Copy link
Contributor

Choose a reason for hiding this comment

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

DecodeInPlace should take beginPath/endPath iterators, modify the bytes in-place to unescape % sequences, and return the new endPath position rather than copy to an intermediate char[]. This also means the DecodeInPlace method does not need to understand utf-8 sequences, because the subsequent conversion from byte[] to string via UTF8 encoder will take care of that concern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To achieve the goal of in place updating in the memory pool block I need to significantly change the UrlPathDecoder so that it accepts two iterator like objects to operator. The existing logic heavily depends on random access for verify UTF8 coding.

I think it's do doable but we won't be able to share the Decoder since it will be highly specified for Kestrel. And I'm more worried about progress at this point to redo this decoder at this point. I'm more than willing to make it happen after rc1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All right. I'm working out a prototype on this idea now. I'll send a separate PR for that. But I'd like to keep this floating as a fall back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the prototype: #274

@troydai
Copy link
Contributor Author

troydai commented Oct 22, 2015

Replaced by #274

@troydai troydai closed this Oct 22, 2015
@troydai troydai deleted the trdai/kestrel124 branch October 23, 2015 22:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants