current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I am working on uploading files through Rest API. Tried my hand using single upload and works. I have modified to accommodate parallel uploads using Futures.

  1. Will there be any chance that this code will not end during the equivalent infinite loop in checking futures's status?

  2. Another question that I have here is that the response does not return any more information about the files. I do not have control over how the server returns. How can I name a more detailed exception during response handling:

    'File upload failed for file xxx! Status code, error message'

    instead of the below?

private void fileUpload(String localFolderPath, String uploadFolderPath) throws ClientProtocolException, IOException,
            ParserConfigurationException, SAXException
    {

    File folder = new File(localFolderPath);
    String url = baseUrl + "/fileUpload";
    FutureRequestExecutionService requestExecService = null;
    ExecutorService execService = Executors.newFixedThreadPool(NTHREDS);
    try
    {
        HttpClient httpclient = HttpClientBuilder.create().setMaxConnPerRoute(NTHREDS)
                .setMaxConnTotal(NTHREDS).build();
        requestExecService = new FutureRequestExecutionService(httpclient, execService);

        ResponseHandler<String> handler = new ResponseHandler<String>() {
            @Override
            public String handleResponse(HttpResponse response) throws ClientProtocolException, IOException
            {
                int statusCode = response.getStatusLine().getStatusCode();
                if (statusCode == HttpStatus.SC_OK)
                {
                    Document xmlDocument;
                    try
                    {
                        String handleResponse = new BasicResponseHandler().handleResponse(response);
                        System.out.println("Response: " + handleResponse);
                        xmlDocument = getXMLDocument(handleResponse);
                        Element documentElement = xmlDocument.getDocumentElement();
                        int errorCode = Integer.parseInt(documentElement.getAttribute("errorCode"));
                        if (errorCode != 200)
                        {
                            String errorString = documentElement.getAttribute("errorString");
                            throw new ClientProtocolException("Error under upload! Error code: " + errorCode
                                    + "\nError String: " + errorString);
                        }
                        else
                            return "";
                    }
                    catch (ParserConfigurationException | SAXException e)
                    {
                        e.printStackTrace();
                    }
                    return null;
                }
                else
                    throw new ClientProtocolException("Failed under upload! Status code: " + statusCode
                            + "\nReason: " + response.getStatusLine().getReasonPhrase());
            }
        };

        List<HttpRequestFutureTask<String>> futureList = new ArrayList<HttpRequestFutureTask<String>>();

        for (File fileEntry : folder.listFiles())
        {
            String fileName = fileEntry.getName();
            long fileSize = fileEntry.length();
            long lastModified = fileEntry.lastModified() / 1000L;
            System.out.println("Uploading " + fileName);

            HttpPost post = new HttpPost(url);
            post.setHeader("Cred", "Token");
            post.setHeader("UploadFolderPath",
                    Base64.encodeBase64String(uploadFolderPath).getBytes()));
            post.setHeader("FileName", Base64.encodeBase64String(fileName.getBytes()));
            post.setHeader("FileSize", Long.toString(fileSize));
            post.setHeader("FileModifiedtime", Long.toString(lastModified));
            post.setEntity(new ByteArrayEntity(IOUtils.toByteArray(new FileInputStream(fileEntry))));

            HttpRequestFutureTask<String> futureTask = requestExecService.execute(post, HttpClientContext.create(), handler);
            futureList.add(futureTask);
        }

        System.out.println("Waiting for results");
        while (futureList.size() > 0)
        {
            System.out.println("FutureList size: " + futureList.size());
            for (Iterator<HttpRequestFutureTask<String>> iterator = futureList.iterator(); iterator.hasNext();)
            {
                try
                {
                    HttpRequestFutureTask<String> futureTask = iterator.next();
                    if (futureTask.get() == null || futureTask.get().length() == 0)
                        iterator.remove();
                }
                catch (InterruptedException | ExecutionException e)
                {
                    e.printStackTrace();
                }
            }
        }
    }
    finally
    {
        if (requestExecService != null)
            requestExecService.close();
        execService.shutdown();
    }
}
share|improve this question

1 Answer 1

up vote 1 down vote

I'm just going to point out one significant issue because I believe it will require a re-think on your implementation....

When you use an ExecutorService it manages a number of threads, and ensures that threads are busy working in a way that's relatively easy to control. You have a system set up where the intention is to have a pool of threads managed in a way that allows multiple uploads to happen at the same time, and to limit the concurrency to NTHREDS (which should possibly be changed to NTHREADS).

Unfortunately, your intentions are not represented in your code. You don't have those controls at all, and your attempts to control the concurrency, and even the attempt to be asynchronous, are all thwarted because you create the ExecutorService and FutureRequestExecutionService inside the method call. Every time you call the fileUpload function you create a complete new environment that is independent from other calls. There is no sharing of anything, and no control... and only one of the threads in each thread pool will be used.

You probably want to do the following: remove the two initializers from the method, and instead make them private fields, perhaps even static, depending on the class the fileUpload method is in.... Then, because you cannot control the orderly shutdown of the thread pool, you probably need to have some form of controlled close method, or you need to ensure all your threads are Daemon threads.... I would consider something like:

private static final Thread buildThread(Runnable runnable) {
    Thread t = new Thread(runnable, "FileUpload thread");
    t.setDaemon(true);
    return t;
}

private static final HttpClient httpclient = HttpClientBuilder.create()
            .setMaxConnPerRoute(NTHREDS)
            .setMaxConnTotal(NTHREDS)
            .build();
private static final ExecutorService execService =
            Executors.newFixedThreadPool(NTHREDS, run -> buildThread(run));
private static final FutureRequestExecutionService requestExecService =
            new FutureRequestExecutionService(httpclient, execService);

With the above code there will likely be some poblems with thrown exeptions... perhaps putting that as a field initialization in a constructor (instead of statics) would be better, because exception handling in a constructor is better.

Regardless, with the above, at least you are managing your connection right.

share|improve this answer
    
Thanks for the suggestion. I've made the ExecutorService, FutureRequestExecutionService, futureList and the ResponseHandler as private fields inside the class. In short, I traverse through every directory under a path, upload files in parallel. Parallelism was under every directory but your suggestion made it parallel under every files. –  dmachop Jun 11 at 18:32

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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