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

Proposal: Form parsing improvements #413

Closed
wants to merge 8 commits into from
Closed

Proposal: Form parsing improvements #413

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a1440dc
Form parsing improvements
benaadams Sep 22, 2015
5e84962
Add comment
benaadams Sep 22, 2015
6c908d7
Clear cached task when setting value directly
benaadams Sep 22, 2015
22f2e98
Array.Empty<string> for DNXCORE50
benaadams Sep 22, 2015
44fb6c5
Don't create array when only one string value
benaadams Sep 22, 2015
4354dbb
CR use section ContentDisposition and ContentType
benaadams Sep 22, 2015
eb4ac33
Reduce boxing, Pre-box Empty enumerators
benaadams Sep 24, 2015
871b9f9
Avoid delegate allocation
benaadams Sep 29, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 57 additions & 10 deletions src/Microsoft.AspNet.Http/Features/FormFeature.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ namespace Microsoft.AspNet.Http.Features.Internal
public class FormFeature : IFormFeature
{
private readonly HttpRequest _request;
private Task<IFormCollection> _parsedFormTask;
private IFormCollection _form;

public FormFeature(IFormCollection form)
{
Expand Down Expand Up @@ -63,7 +65,15 @@ public bool HasFormContentType
}
}

public IFormCollection Form { get; set; }
public IFormCollection Form
{
get { return _form; }
set
{
_parsedFormTask = null;
Copy link
Member

Choose a reason for hiding this comment

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

Why not set the cached task here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You've got the non-async method; so didn't want to set the task if that's the only path used

_form = value;
}
}

public IFormCollection ReadForm()
{
Expand All @@ -77,17 +87,32 @@ public IFormCollection ReadForm()
throw new InvalidOperationException("Incorrect Content-Type: " + _request.ContentType);
}

// TODO: Avoid Sync-over-Async http://blogs.msdn.com/b/pfxteam/archive/2012/04/13/10293638.aspx
// TODO: How do we prevent thread exhaustion?
return ReadFormAsync(CancellationToken.None).GetAwaiter().GetResult();
return ReadFormAsync().GetAwaiter().GetResult();
}

public async Task<IFormCollection> ReadFormAsync(CancellationToken cancellationToken)
public Task<IFormCollection> ReadFormAsync() => ReadFormAsync(CancellationToken.None);

public Task<IFormCollection> ReadFormAsync(CancellationToken cancellationToken)
{
if (Form != null)
// Avoid state machine and task allocation for repeated reads
if (_parsedFormTask == null)
{
return Form;
if (Form != null)
{
_parsedFormTask = Task.FromResult(Form);
}
else
{
_parsedFormTask = InnerReadFormAsync(cancellationToken);
}
}
return _parsedFormTask;
}

private async Task<IFormCollection> InnerReadFormAsync(CancellationToken cancellationToken)
{
if (!HasFormContentType)
{
throw new InvalidOperationException("Incorrect Content-Type: " + _request.ContentType);
Expand All @@ -98,7 +123,7 @@ public async Task<IFormCollection> ReadFormAsync(CancellationToken cancellationT
_request.EnableRewind();

IDictionary<string, StringValues> formFields = null;
var files = new FormFileCollection();
FormFileCollection files = null;

// Some of these code paths use StreamReader which does not support cancellation tokens.
using (cancellationToken.Register(_request.HttpContext.Abort))
Expand All @@ -119,7 +144,7 @@ public async Task<IFormCollection> ReadFormAsync(CancellationToken cancellationT
var section = await multipartReader.ReadNextSectionAsync(cancellationToken);
while (section != null)
{
var headers = new HeaderDictionary(section.Headers);
var headers = section.Headers;
ContentDispositionHeaderValue contentDisposition;
ContentDispositionHeaderValue.TryParse(headers[HeaderNames.ContentDisposition], out contentDisposition);
Copy link
Member

Choose a reason for hiding this comment

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

regression: this will throw if Content-Disposition is missing. You could just use section.ContentDisposition instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which would wrap; so delay wrapping down a few lines for FormFile wouldn't help. Will revert.

Copy link
Member

Choose a reason for hiding this comment

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

That wouldn't wrap: https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNet.WebUtilities/MultipartSection.cs#L25-L38

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed as suggested

if (HasFileContentDisposition(contentDisposition))
Expand All @@ -129,8 +154,12 @@ public async Task<IFormCollection> ReadFormAsync(CancellationToken cancellationT

var file = new FormFile(_request.Body, section.BaseStreamOffset.Value, section.Body.Length)
{
Headers = headers,
Headers = new HeaderDictionary(headers),
};
if (files == null)
{
files = new FormFileCollection();
}
files.Add(file);
}
else if (HasFormDataContentDisposition(contentDisposition))
Expand All @@ -157,14 +186,32 @@ public async Task<IFormCollection> ReadFormAsync(CancellationToken cancellationT
section = await multipartReader.ReadNextSectionAsync(cancellationToken);
}

formFields = formAccumulator.GetResults();
if (formAccumulator.HasValues)
{
formFields = formAccumulator.GetResults();
}
}
}

// Rewind so later readers don't have to.
_request.Body.Seek(0, SeekOrigin.Begin);

Form = new FormCollection(formFields, files);
if (formFields == null || formFields.Count == 0)
{
if (files == null || files.Count == 0)
{
Form = FormCollection.Empty;
}
else
{
Form = new FormCollection(files);
}
}
else
{
Form = new FormCollection(formFields, files);
}

return Form;
}

Expand Down
33 changes: 27 additions & 6 deletions src/Microsoft.AspNet.Http/FormCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,24 @@ namespace Microsoft.AspNet.Http.Internal
/// </summary>
public class FormCollection : ReadableStringCollection, IFormCollection
{
public static new readonly IFormCollection Empty = new FormCollection();

private static readonly IFormFileCollection EmptyFiles = new FormFileCollection();
private IFormFileCollection _files;

private FormCollection()
: base()
{
// For static Empty
}

internal FormCollection(IFormFileCollection files)
: base()
{
Files = files;
// For empty parsed data, with files (can be null)
}

public FormCollection(IDictionary<string, StringValues> store)
: this(store, new FormFileCollection())
{
Expand All @@ -25,14 +43,17 @@ public FormCollection(IDictionary<string, StringValues> store, IFormFileCollecti
throw new ArgumentNullException(nameof(store));
}

if (files == null)
{
throw new ArgumentNullException(nameof(files));
}

// Files can be null
Files = files;
}

public IFormFileCollection Files { get; }
public IFormFileCollection Files
{
get
{
return _files ?? EmptyFiles;
}
private set { _files = value; }
}
}
}
51 changes: 48 additions & 3 deletions src/Microsoft.AspNet.Http/ReadableStringCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using Microsoft.Framework.Primitives;

namespace Microsoft.AspNet.Http.Internal
Expand All @@ -13,7 +14,16 @@ namespace Microsoft.AspNet.Http.Internal
/// </summary>
public class ReadableStringCollection : IReadableStringCollection
{
public static readonly IReadableStringCollection Empty = new ReadableStringCollection(new Dictionary<string, StringValues>(0));
public static readonly IReadableStringCollection Empty = new ReadableStringCollection();
#if !DNXCORE50
private static readonly string[] EmptyKeys = new string[0];
#endif
private static readonly IEnumerable<KeyValuePair<string, StringValues>> EmptyEnumerable = new KeyValuePair<string, StringValues>[0];

internal ReadableStringCollection()
{
// For static Empty and empty parsed data
}

/// <summary>
/// Create a new wrapper
Expand All @@ -36,15 +46,35 @@ public ReadableStringCollection(IDictionary<string, StringValues> store)
/// </summary>
public int Count
{
get { return Store.Count; }
get
{
if (Store == null)
{
return 0;
}

return Store.Count;
}
}

/// <summary>
/// Gets a collection containing the keys.
/// </summary>
public ICollection<string> Keys
{
get { return Store.Keys; }
get
{
if (Store == null)
{
#if DNXCORE50
return Array.Empty<string>();
#else
return EmptyKeys;
#endif
}

return Store.Keys;
}
}


Expand All @@ -58,6 +88,11 @@ public StringValues this[string key]
{
get
{
if (Store == null)
{
return StringValues.Empty;
}

StringValues value;
if (Store.TryGetValue(key, out value))
{
Expand All @@ -74,6 +109,11 @@ public StringValues this[string key]
/// <returns></returns>
public bool ContainsKey(string key)
{
if (Store == null)
{
return false;
}

return Store.ContainsKey(key);
}

Expand All @@ -84,6 +124,11 @@ public bool ContainsKey(string key)
/// <returns></returns>
public IEnumerator<KeyValuePair<string, StringValues>> GetEnumerator()
{
if (Store == null)
{
return EmptyEnumerable.GetEnumerator();
}

return Store.GetEnumerator();
}

Expand Down
30 changes: 22 additions & 8 deletions src/Microsoft.AspNet.WebUtilities/KeyValueAccumulator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,16 @@

namespace Microsoft.AspNet.WebUtilities
{
public class KeyValueAccumulator
public struct KeyValueAccumulator
Copy link
Member

Choose a reason for hiding this comment

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

Does this even need an inner dictionary? It could be just have a callback OnKeyValuePair(string key, string value) or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could do but would need the allocation of an outer dictionary, a closure; or some code rearranging? For example its used in the extension method QueryHelpers.ParseQuery

You can avoid alloc with these changes by checking HasValues before calling GetResults() as is done in the FormFeature.InnerReadFormAsync

{
private Dictionary<string, List<string>> _accumulator;

public KeyValueAccumulator()
{
_accumulator = new Dictionary<string, List<string>>(StringComparer.OrdinalIgnoreCase);
}

public void Append(string key, string value)
{
if (_accumulator == null)
{
_accumulator = new Dictionary<string, List<string>>(StringComparer.OrdinalIgnoreCase);
}
List<string> values;
if (_accumulator.TryGetValue(key, out values))
{
Expand All @@ -29,12 +28,27 @@ public void Append(string key, string value)
}
}

public bool HasValues => _accumulator != null;

public IDictionary<string, StringValues> GetResults()
{
var results = new Dictionary<string, StringValues>(StringComparer.OrdinalIgnoreCase);
if (_accumulator == null)
{
return new SafeLazyDictionary<StringValues>();
}

var results = new SafeLazyDictionary<StringValues>(_accumulator.Count);

foreach (var kv in _accumulator)
{
results.Add(kv.Key, kv.Value.ToArray());
if (kv.Value.Count == 1)
{
results.Add(kv.Key, new StringValues(kv.Value[0]));
}
else
{
results.Add(kv.Key, new StringValues(kv.Value.ToArray()));
}
}
return results;
}
Expand Down
Loading