Skip to content
This repository has been archived by the owner on Sep 9, 2022. It is now read-only.

Commit

Permalink
Fixed #20
Browse files Browse the repository at this point in the history
  • Loading branch information
@azzlack
azzlack committed Jan 28, 2016
1 parent 4d5b78a commit 0325fad
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 29 deletions.
16 changes: 8 additions & 8 deletions src/Server/Attributes/CompressionAttribute.cs
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
namespace Microsoft.AspNet.WebApi.Extensions.Compression.Server.Attributes
{
using System.Threading;
using System.Threading.Tasks;
using System.Web.Http.Controllers;
using System.Web.Http.Filters;

public class CompressionAttribute : ActionFilterAttribute
{
/// <summary>
/// Initializes a new instance of the Microsoft.AspNet.WebApi.MessageHandlers.Compression.Attributes.CompressionAttribute class.
/// </summary>
/// <summary>Initializes a new instance of the <see cref="CompressionAttribute" /> class.</summary>
public CompressionAttribute()
{
this.Enabled = true;
Expand All @@ -17,15 +17,15 @@ public CompressionAttribute()
/// <value><c>true</c> if enabled, <c>false</c> if not. The default value is <c>false</c>.</value>
public bool Enabled { get; set; }

/// <summary>
/// Occurs before the action method is invoked.
/// </summary>
/// <summary>Executes the action executing asynchronous action.</summary>
/// <param name="actionContext">The action context.</param>
public override void OnActionExecuting(HttpActionContext actionContext)
/// <param name="cancellationToken">The cancellation token.</param>
/// <returns>A Task.</returns>
public override Task OnActionExecutingAsync(HttpActionContext actionContext, CancellationToken cancellationToken)
{
actionContext.Request.Properties.Add("compression:Enable", this.Enabled);

base.OnActionExecuting(actionContext);
return base.OnActionExecutingAsync(actionContext, cancellationToken);
}
}
}
29 changes: 8 additions & 21 deletions src/Server/ServerCompressionHandler.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
{
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Linq;
using System.Net.Http;
using System.Net.Http.Extensions.Compression.Core;
Expand Down Expand Up @@ -128,32 +127,20 @@ protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage
await this.DecompressRequest(request);
}

// Perform original action
var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false);

// Check if response should be compressed
// NOTE: This must be done _after_ the response has been created, otherwise the EnableCompression property is not set
var process = this.enableCompression(request);

try
{
if (process && response.Content != null)
{
// Buffer content for further processing
await response.Content.LoadIntoBufferAsync();
}
else
{
process = false;
}
}
catch (Exception ex)
{
process = false;

Debug.WriteLine(ex.Message);
}

// Compress uncompressed responses from the server
// Compress content if it should processed
if (process && response.Content != null)
{
// Buffer content for further processing.
// NOTE: This is needed to properly calculate Content-Length
await response.Content.LoadIntoBufferAsync();

This comment has been minimized.

Sign in to view

Copy link
@ahmedalejo

ahmedalejo Aug 26, 2016

This is absolutely unnecessary, flawed and causes duplications of memory streams in the following places

   using (var gzip = this.CreateCompressionStream(mem))
   {
            await source.CopyToAsync(gzip);
    }

in BaseCompressor .Compress line 47 to 50

  • with
var compressed = new byte[mem.Length];
await mem.ReadAsync(compressed, 0, compressed.Length);

in BaseCompressor .Compress on line 54 and line 55

  • With
var outStream = new MemoryStream(compressed);
await outStream.CopyToAsync(destination);

in BaseCompressor .Compress on line 57 and line 58

  • with
using (var ms = new MemoryStream(await this.originalContent.ReadAsByteArrayAsync()))
{
    var compressedLength = await this.compressor.Compress(ms, stream).ConfigureAwait(false);

    // Content-Length: {size}
    this.Headers.ContentLength = compressedLength;
}

in CompressedContent.SerializeToStreamAsync on line 75 and line 81

This is a greatly flawed implementation that we need to fix.

For small contents, the server memory footprint negligible, but for a 10 MB file, approximately 30 MB of data is held in memory before sending to response stream.

This hinders file streaming, and may not even work for bigger files ex. 100MB

Proposed Fixes

var compressed = this.CreateCompressionStream(destination);

 return this.Pump(source, compressed)
       .ContinueWith(task => compressed.Dispose());
  • await response.Content.LoadIntoBufferAsync(); should be removed (or only used small files
    if size is computable)

and

using (_content)
 {
    var contentStream = await this.originalContent.ReadAsStreamAsync();
    await _compressor.Compress(contentStream, stream);
}

Will give huge performance boost, both on memory usage and latency(due to buffering)

await this.CompressResponse(request, response);
}

Expand Down

0 comments on commit 0325fad

Please sign in to comment.