Uploading a video in S3 Using Future

Question

I've posted a similar program previously, but I have not made any major modifications in the code, so I am posting it again by deleting the previous question.

I am afraid of the thread keyword and I am not too familiar with threads and blocking, so can you please review this code for me?

Is the use of future enough or not?

This code comes from here.

Note: The AWS credentials are fake.

package controllers
import java.awt.image.BufferedImage
import java.util.ArrayList
import scala.collection.JavaConversions.mapAsJavaMap
import scala.concurrent.Future
import org.apache.http.NameValuePair
import org.apache.http.message.BasicNameValuePair
import com.amazonaws.auth.BasicAWSCredentials
import com.amazonaws.services.s3.AmazonS3Client
import com.twilio.sdk.TwilioRestClient
import com.twilio.sdk.TwilioRestException
import net.liftweb.json.DefaultFormats
import net.liftweb.json.Serialization.write
import play.api.Play.current
import play.api.i18n.Lang
import play.api.i18n.Messages
import play.api.libs.concurrent.Execution.Implicits._
import play.api.mvc.Action
import play.api.mvc.Controller
object Application extends Controller {
 val bucketImages = "images35"
 val bucketVideos = "videos35"
 val bucketAudios = "audios35"
 val AWS_ACCESS_KEY = "qqqqqqqqqqqqqqqqqqqqqqqqqqq"
 val AWS_SECRET_KEY = "xxxxxxxxxxxxxxxxxxxxxxxxxxx"
 val yourAWSCredentials = new BasicAWSCredentials(AWS_ACCESS_KEY, AWS_SECRET_KEY)
 val amazonS3Client = new AmazonS3Client(yourAWSCredentials)
 val ACCOUNT_SID = "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
 val AUTH_TOKEN = "tytytytytyyyyyyyyyyyyyyyyyyyyyyyyyy"
 implicit val formats = DefaultFormats
 def uploadVideo = Action.async(parse.multipartFormData) { implicit request =>
session.get("userI").map { userId =>
 try {
 var videoName: String = null
 val upload = for {
 done <- Future {
 request.body.files.map { mov =>
 videoName =System.currentTimeMillis() + ".mpeg"
 amazonS3Client.putObject(bucketVideos, videoName, mov.ref.file)
 }
 }
 } yield done
 upload.map { res =>
 val map = Map("result" -> "success", "videoName" -> videoName)
 Ok(write(map))
 }
 } catch {
 case e: Exception =>
 Future{
 Ok(write(Map("result" -> "error")))
 }
 }
}.getOrElse {
 Future{
 Ok(write(Map("result" -> "nosession")))
 }
 }
 }
}

application.conf

play {
 akka {
 event-handlers = ["akka.event.slf4j.Slf4jEventHandler"]
 loglevel = WARNING
 actor {
 default-dispatcher = {
 fork-join-executor {
 parallelism-min = 300
 parallelism-max = 300
 }
 }
 }
 }
}
contexts {
 simple-db-lookups {
 fork-join-executor {
 parallelism-factor = 10.0
 }
 }
 expensive-db-lookups {
 fork-join-executor {
 parallelism-max = 4
 }
 }
 db-write-operations {
 fork-join-executor {
 parallelism-factor = 2.0
 }
 }
 expensive-cpu-operations {
 fork-join-executor {
 parallelism-max = 2
 }
 }
}

Contexts Object

object Contexts {
 implicit val simpleDbLookups: ExecutionContext = Akka.system.dispatchers.lookup("contexts.simple-db-lookups")
 implicit val expensiveDbLookups: ExecutionContext = Akka.system.dispatchers.lookup("contexts.expensive-db-lookups")
 implicit val dbWriteOperations: ExecutionContext = Akka.system.dispatchers.lookup("contexts.db-write-operations")
 implicit val expensiveCpuOperations: ExecutionContext = Akka.system.dispatchers.lookup("contexts.expensive-cpu-operations")
}

Answer 1

First, remove import play.api.libs.concurrent.Execution.Implicits._. You don't want the default Play execution context. Instead, you should use one of the ExecutionContexts you've defined and aren't using. Perhaps define one for S3 operations alone, and you don't need the others if you aren't going to use them. Future.apply requires the implicit ExecutionContext, so you can either import the correct one, or explicitly pass it.

Assuming you have a context defined like this in your configuration:

contexts {
 s3-ops {
 fork-join-executor {
 parallelism-factor = 10.0
 }
 }
}

Then in your controller you could add:

implicit val ec: ExecutionContext = Akka.system.dispatchers.lookup("contexts.s3-ops")

Now in the body of the controller function, I'll work my way inward. With the handling of the session, why bother returning data to the user if they don't have a session? It would seem more appropriate to return Forbidden, which can shorten that line to Future.successful(Forbidden). Note that Future.successful creates a Future that has already been completed.

The use of try/catch is not very Scala-like. There are other nicer ways to handle errors in Scala like Try. Future is actually very similar to Try, though, and it should provide all the error handling you need in this case. If an exception occurs within a Future, then it becomes a failed Future. No exceptions will propagate upward. Most of the code inside your try block can go inside the Future apply instead.

var videoName: String = null is something that should be avoided altogether. First, it's best to avoid mutable vars, and second you should never find yourself assigning null to anything.

I'm not sure the body of your current Future where you're mapping request.body.files is doing what you think it's doing. request.body.files is a Seq[FilePart], which means if there are multiple FileParts, you're going to upload them all separately, overwrite videoName and only return the name of the last uploaded FilePart. Or worse, if there are no FileParts, that part will silently fail. If you're only going to have a single FilePart, it'd be better to name it in the upload body and access via request.body.file("filename") which will be an Option[FilePart]. From there you can map the Option[FilePart] to the actual upload.

Since you're not actually doing anything with the PutObjectResult from the S3 response, it'd be better to return the name of the file in the Future, so that you have access to it when you map the Future, and would no longer have a need for a var. Following the suggestions above, it would now be a Future[Option[String]].

This would leave you with three return states for the Future:

1. A successful Future containing Some[String] (upload success on both ends).
2. A successful Future containing None (no file was uploaded to the server).
3. A failed Future (the upload to S3 failed somehow).

You'd then have something that looks roughly like this:

Future {
 request.body.file("filename").map{ file =>
 val videoName = ...
 amazonS3Client.putObject(...)
 videoName
 }
}.map{ upload =>
 upload.map{videoName => Ok(...)}.getOrElse{BadRequest(...)}
}.recover{ case _ => // recover the failed `Future` to a default value, in this case a `Result`
 InternalServerError
}

Much cleaner. Note that recover accepts a PartialFunction[Throwable, ?], similarly to a catch block, so you can be more fine grained in your error handling if you need to.

A couple more notes:

There's no need to use Lift's json API for your responses. Play's implementation works just fine:

import play.api.libs.json.Json
Ok(Json.obj("result" -> "success", "videoName" -> videoName))

Your imports take up a lot of space. Many can be condensed into one liners:

 import play.api.i18n.Lang
 import play.api.i18n.Messages

Can be:

 import play.api.i18n.{Lang, Messages}