0
\$\begingroup\$

I've been experimenting with some old code that needs refactoring in places and was testing if there was any improvement to iis threads etc by uploading file asynchronously (Server Side). I'm using jQuery file upload client side.

The original code

[HttpPost]
public ActionResult UploadDocument( HttpPostedFileBase uploadedFile ) {

  // Do any validation here

  // Read bytes from http input stream into fileData
  Byte[] fileData;

  using ( BinaryReader binaryReader = 
          new BinaryReader( uploadedFile.InputStream ) ) {

    fileData = binaryReader.ReadBytes( uploadedFile.ContentLength );

  }

  // Create a new Postgres bytea File Blob ** NOT Async **
  _fileService.CreateFile( fileData );

  return Json(
    new {
      ReturnStatus = "SUCCESS" // Or whatever
    }
  );

}

The new code

[HttpPost]
public async Task<ActionResult> UploadDocumentAsync( HttpPostedFileBase uploadedFile ) {

  // Do any validation here

  // Read bytes from http input stream into fileData
  Byte[] fileData = new Byte[uploadedFile.ContentLength];

  await uploadedFile.InputStream.ReadAsync( fileData, 0, uploadedFile.ContentLength );

  // Create a new Postgres bytea File Blob ** NOT Async **
  _fileService.CreateFile( fileData );

  return Json(
    new {
      ReturnStatus = "SUCCESS" // Or whatever
    }
  );

}

The new method appears to work correctly but my question is:

Is the following code the correct (best) way to do it? and are there any gotchas doing it this way? There is a lot of contradictory and out of date information out there. There also seems to be a lot of debate on whether there is any improvement or point in actually doing this. Yes it give back threads to iis etc but is it worth the overhead type of debate.

The code in question

// Read bytes from http input stream into fileData
Byte[] fileData = new Byte[uploadedFile.ContentLength];

await uploadedFile.InputStream.ReadAsync( fileData, 0, uploadedFile.ContentLength );
\$\endgroup\$
2
\$\begingroup\$

Your proposed changes will have marginal impact. The way IIS works, IHttpHandlers are not invoked until the entire request body has been transferred. That is, the entirety of the file data is already available by the time your first line of code runs. That means your stream copying is just moving bytes around in memory, and while that is not instantaneous it's probably not slow enough that it's worth making it asynchronous.

I think you already know this, but you should make the Postgres call asynchronous if that's an option. With the current call, you have a thread taken up waiting for a remote process to do its work. With an asynchronous call, that thread would be available to do real work. Of course, that only makes a difference if there is an incoming request that's waiting for a thread.

\$\endgroup\$
1
\$\begingroup\$

This is essentially right. I have had issues with not being able to read the entire stream (many streams I have worked with only return chunks of data for whatever reason so I've learned to not trust Read or related functions without checking the results) in one go so I would suggest something like this:

byte[] fileData;
using (var ms = new MemoryStream()) {
    await uploadedFile.InputStream.CopyToAsync(ms);
    fileData = ms.ToArray();
}

The bigger issue here is that I am not sure if reading this stream async will get you anything. The blocking operation here is the out of process request to write the data to postgres. I am not sure a controller is ever waiting for input data.

It comes down to considering what async is doing for you. async allows you to take an already blocking operation you do not have control over (a network request for example) and free your thread to do something else while that is waiting for a response. This means that if your application was sitting at 100 requests before async and most of those requests are spending most of their time waiting for the database to respond, you could switch to async code around those database requests and now all of that time you were sitting around waiting can instead be used to queue up more requests from the clients.

\$\endgroup\$
  • \$\begingroup\$ stackoverflow.com/questions/29433842/… \$\endgroup\$ – William Humphreys Apr 3 '15 at 17:50
  • \$\begingroup\$ eh. usr has the right answer there I think. The original code is fine. You CAN mix async and sync IO like you are doing so the resulting code is OK (provided you are operating the stream contract correctly which I am saying you are not doing exactly correct even though I don't think it matters in this case), but file uploads are already essentially not blocking IO so reading them async doesn't really do anything for you. \$\endgroup\$ – Bill Barry Apr 3 '15 at 19:09
  • \$\begingroup\$ If instead of doing the file upload async you did the database write async, you would be releasing this resource (the thread active for this method) for the duration of that network request. That is what async gives you (the ability to do something else while you are waiting for a resource not in your control). \$\endgroup\$ – Bill Barry Apr 3 '15 at 19:09
  • \$\begingroup\$ Further, just because file uploads aren't necessarily great candidates for being async today, doesn't mean they wouldn't be under some future version of this code. This async use doesn't hurt your code. I probably wouldn't make the async change here in my code, but if I was writing the method new I might write it async the first time. \$\endgroup\$ – Bill Barry Apr 3 '15 at 19:14
  • \$\begingroup\$ From my research (that may be wrong) You dont really gain anything from using Async for database updates especially local ones in fact the overhead to make it Async is usually more than you gain. \$\endgroup\$ – William Humphreys Apr 3 '15 at 21:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.