Use try-with-resources or close this "BufferedInputStream" in a "finally" clause

Use try-with-resources or close this "BufferedInputStream" in a "finally" clause.

DB Connection, InputStream, file, URLConnection 등 Closeable 인터페이스를 구현 또는 상속받은 클래스는 반드시 finally에서 해당 자원을 close 해줘야만 합니다. 

테스트 시점에는 아무 에러가 없었다 하더라도 시간이 지나서 아무런 이유없이 시스템이 다운되거나 하는 경우를 볼 수 있습니다. 

개발자가 놓치기 쉬운 부분이지만 매우 중요한 부분입니다. 

이런 부분은 반드시 수정해야만 합니다.

수정 전 

    public void fileDownload(String filePath, String filename, HttpServletResponse response){
        try {
            File f = new File(filePath+filename);
            if (!f.exists()) {
                log.error("error"); // 존재하지 않는 파일 다운로드
                return;
            }
 
            filename = new String(filename.getBytes("euc-kr"), "8859_1");
            response.setContentType("application/octet-stream");
            response.setHeader("Content-disposition", "attachment;filename="+ filename);
 
            // 파일 내용을 클라이언트에 전송
            byte[] b = new byte[1024];
            BufferedInputStream bis = new BufferedInputStream(new FileInputStream(f));
            OutputStream os = response.getOutputStream();
 
            int n;
            while ((n = bis.read(b, 0, b.length)) != -1) { // 파일을 모두 읽으면 -1리턴
                os.write(b, 0, n);
            }
            os.flush();
            os.close();
            bis.close();
        } catch (Exception e) {
            e.printStackTrace();
        }
    }

수정 후 

    public void fileDownload(String filePath, String filename, HttpServletResponse response){
        BufferedInputStream bis = null;
        OutputStream os = null;
        try {
            File f = new File(filePath+filename);
            if (!f.exists()) {
                log.error("error"); // 존재하지 않는 파일 다운로드
                return;
            }
 
            filename = new String(filename.getBytes("euc-kr"), "8859_1");
            response.setContentType("application/octet-stream");
            response.setHeader("Content-disposition", "attachment;filename="+ filename);
 
            // 파일 내용을 클라이언트에 전송
            byte[] b = new byte[1024];
            bis = new BufferedInputStream(new FileInputStream(f));
            os = response.getOutputStream();
 
            int n;
            while ((n = bis.read(b, 0, b.length)) != -1) { // 파일을 모두 읽으면 -1리턴
                os.write(b, 0, n);
            }
            os.flush();
        } catch (Exception e) {
            e.printStackTrace();
        } finally {
            if(os != null) {
                try {
                    os.close();
                } catch (IOException e) {
                    log.error(e.getMessage());
                }
            }
            if(bis != null) {
                try {
                    bis.close();
                } catch (IOException e) {
                    log.error(e.getMessage());
                }
            }
        }
    }
 

최종 수정 후 

    public void fileDownload(String filePath, String filename, HttpServletResponse response){
        File f = new File(filePath+filename);
        if (!f.exists()) {
            log.error("파일이 존재하지않습니다."); // 존재하지 않는 파일 다운로드
            return;
        }
        filename = new String(filename.getBytes(StandardCharsets.ISO_8859_1));
        response.setContentType("application/octet-stream");
        response.setHeader("Content-disposition", "attachment;filename="+ filename);
        try(
                BufferedInputStream bis = new BufferedInputStream(new FileInputStream(f)); 
                OutputStream os=response.getOutputStream();
            ) {
            byte[] b = new byte[1024];
            int n;
            while ((n = bis.read(b, 0, b.length)) != -1) {
                os.write(b, 0, n);
            }
            os.flush();
        } catch (Exception e) {
            log.error(e.getMessage());
        }
    }
 

Resources should be closed (squid:S2095)

Connections, streams, files, and other classes that implement the Closeable interface or its super-interface, AutoCloseable, needs to be closed after use. Further, that close call must be made in a finally block otherwise an exception could keep the call from being made. Preferably, when class implements AutoCloseable, resource should be created using "try-with-resources" pattern and will be closed automatically.

Failure to properly close resources will result in a resource leak which could bring first the application and then perhaps the box it's on to their knees.

Noncompliant Code Example

private void readTheFile() throws IOException {
  Path path = Paths.get(this.fileName);
  BufferedReader reader = Files.newBufferedReader(path, this.charset);
  // ...
  reader.close();  // Noncompliant
  // ...
  Files.lines("input.txt").forEach(System.out::println); // Noncompliant: The stream needs to be closed
}

private void doSomething() {
  OutputStream stream = null;
  try {
    for (String property : propertyList) {
      stream = new FileOutputStream("myfile.txt");  // Noncompliant
      // ...
    }
  } catch (Exception e) {
    // ...
  } finally {
    stream.close();  // Multiple streams were opened. Only the last is closed.
  }
}

Compliant Solution

private void readTheFile(String fileName) throws IOException {
    Path path = Paths.get(fileName);
    try (BufferedReader reader = Files.newBufferedReader(path, StandardCharsets.UTF_8)) {
      reader.readLine();
      // ...
    }
    // ..
    try (Stream<String> input = Files.lines("input.txt"))  {
      input.forEach(System.out::println);
    }
}

private void doSomething() {
  OutputStream stream = null;
  try {
    stream = new FileOutputStream("myfile.txt");
    for (String property : propertyList) {
      // ...
    }
  } catch (Exception e) {
    // ...
  } finally {
    stream.close();
  }
}