2
\$\begingroup\$

I am using the following function to upload videos (around 1 gb). It works properly.

My requirements are (these checks are being done in a different function using the mediainfo library):

  • check if the file is a valid video file
  • check video count
  • check audio count
  • check video resolution
  • etc

Is this the right way to do it? How can I improve on this? I am just trying to get my head around this.

func UploadCreative(jwtString string, r *http.Request, w http.ResponseWriter) {
    userUUID, err := settings.GetUserUUIDFromJWT(jwtString)
    methodName := "service.UploadCreative"
    if err != nil {
        settings.LogError(jwt.ErrSignatureInvalid, methodName, "Invalid Token")
        response, _ := json.Marshal(settings.ErrInternalServer)
        settings.WriteResponse(w, http.StatusInternalServerError, response)
        // return http.StatusInternalServerError, response

    }

    // TBD: Send the correct response
    type Response struct {
        Message string `json:"message"`
    }
    // temp folder path
    chunkDirPath := "./creatives/.uploads/" + userUUID
    // create folder
    err = os.MkdirAll(chunkDirPath, 02750)

    if err != nil {
        settings.LogError(err, methodName, "Failed to create folders")
        response, _ := json.Marshal(settings.ErrInternalServer)
        settings.WriteResponse(w, http.StatusInternalServerError, response)
    }

    // Get file handle from multipart request
    var file io.Reader
    mr, err := r.MultipartReader()
    if err != nil {
        settings.LogError(err, methodName, "Error reading form data")
        response, _ := json.Marshal(settings.ErrInternalServer)
        settings.WriteResponse(w, http.StatusInternalServerError, response)
    }

    // Read multipart body until the "file" part
    for {
        part, err := mr.NextPart()
        if err == io.EOF {
            break
        }
        if part.FormName() == "file" {
            file = part
            break
        }
    }

    if file == nil {
        settings.LogError(nil, methodName, "Missing file from multipart request")
        response, _ := json.Marshal(settings.ErrInternalServer)
        settings.WriteResponse(w, http.StatusInternalServerError, response)
    }

    if err != nil {
        settings.LogError(err, methodName, "Error creating file")
        response, _ := json.Marshal(settings.ErrInternalServer)
        settings.WriteResponse(w, http.StatusInternalServerError, response)
    }

    // Pipe file data from the request body to a preprocessing goroutine
    //  - Guess content type
    preprocessReader, preprocessWriter := io.Pipe()
    totalBytes := 0
    quit := make(chan bool)
    continueUpload := make(chan bool)
    errs := make(chan error)
    go func() {
        for {
            select {
            case <-quit:
                fmt.Println("quit")
                preprocessWriter.Close()
                preprocessReader.Close()
                return
            default:
                buf := make([]byte, 1024)
                bytesRead, err := file.Read(buf)
                if err != nil {
                    if err != io.EOF {
                        settings.LogError(err, methodName, "Unable to read data from request body")
                    }

                    preprocessWriter.Close()
                    return
                }

                // Increment size
                totalBytes += bytesRead

                // Pass file data to data backend
                preprocessWriter.Write(buf[:bytesRead])
            }

        }
    }()

    // Create files
    tempFile := chunkDirPath + "/tempFile"
    dst, err := os.Create(tempFile)

    defer dst.Close()

    go func() {
        for {
            select {
            case <-quit:
                preprocessWriter.Close()
                preprocessReader.Close()
                return
            default:
                fileInfo, _ := os.Stat(tempFile)
                fileSize := fileInfo.Size()

                if fileSize > 1024 {
                    ok, _, err := mediaInfo.CheckMedia(tempFile)
                    if ok {
                        continueUpload <- true
                        errs <- nil
                        return
                    }
                    preprocessWriter.Close()
                    preprocessReader.Close()
                    continueUpload <- false
                    errs <- err
                    return
                }
            }
        }

    }()

    // Copy file data from the client request body
    // to the file system
    _, err = io.Copy(dst, preprocessReader)

    validFile := <-continueUpload
    errString := <-errs

    if validFile {
        response, _ := json.Marshal(&Response{"File uploaded successfully"})
        settings.WriteResponse(w, http.StatusOK, response)
    } else {
        response, _ := json.Marshal(&Response{fmt.Sprintf("Upload cancelled: %s", errString.Error())})
        settings.WriteResponse(w, http.StatusOK, response)
    }

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

Error handling

Two of the first issues I see are code duplication and error handling - and they both are the same thing, in this case. Consider this code of yours:

if err != nil {
    settings.LogError(jwt.ErrSignatureInvalid, methodName, "Invalid Token")
    response, _ := json.Marshal(settings.ErrInternalServer)
    settings.WriteResponse(w, http.StatusInternalServerError, response)
    // return http.StatusInternalServerError, response

}

At first glance, that looks OK, but, there are a few problems:

  1. You do not report the actual Error - err is not actually reported somewhere. What if the error was "UUID is invalid: Illegal character 'G' at position 25". That's a useful error message, but instead, the user just gets the error "Invalid Token". In your other error-handling block, you do a better job og logging the actual err value, but those values are still not reported to the "client", they are just logged on the server.
  2. You don't alter the behaviour of the program.... you "report" the problem, but you don't return, or abort the file processing. This is a big deal. For example, when you have mr, err := r.MultipartReader() you report a broken err, but you keep processing things, which means your mr may be nil, and your following code part, err := mr.NextPart() will panic.
  3. Duplication! That same error logic is repeated many times. You should refactor the code and reduce the duplication:

Consider:

func handleError(w http.ResponseWritererr error, methodName, message string) {
    settings.LogError(err, methodName, mesage)
    // find a way to add the err details to the response too!
    response, _ := json.Marshal(settings.ErrInternalServer)
    settings.WriteResponse(w, http.StatusInternalServerError, response)
}

Now, you can replace your error handling like:

err = os.MkdirAll(chunkDirPath, 02750)

if err != nil {
    settings.LogError(err, methodName, "Failed to create folders")
    response, _ := json.Marshal(settings.ErrInternalServer)
    settings.WriteResponse(w, http.StatusInternalServerError, response)
}

with:

if err = os.MkdirAll(chunkDirPath, 02750); err != nil {
    handleError(w, err, methodName, "Failed to create folders")
    return
}

quit

You create quit, but it's dead code - nothing writes to that channel, so there's no point to it.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the inputs. I will do the refactoring. Do you know other better ways of handling large file uploads? Or do you think that the way I am uploading the file is correct (except for refactoring) \$\endgroup\$ – Monodeep Apr 28 '16 at 12:13

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.