2012-04-01 3 views
2

Я просто хотел посмотреть, есть ли лучший способ, которым я должен справиться с этим. Мое понимание потоков заключается в том, что до тех пор, пока вы закрываете поток, любые потоки, инкапсулированные внутри него, будут закрыты, поэтому я окончательно закрываю TarArchiveOutputStream. Если я получаю FileNotFound на rawDir или archiveDir, я хочу его зарегистрировать, иначе все остальное, что я хочу бросить.Java, обработка исключений и закрытие потоков с помощью try, наконец

public static void createTarGzOfDirectory(File rawDir, File archiveFile) throws IOException { 
    FileOutputStream fOut = null; 
    BufferedOutputStream bOut = null; 
    GzipCompressorOutputStream gzOut = null; 
    TarArchiveOutputStream tOut = null; 
    try { 
     fOut = new FileOutputStream(archiveFile); 
     bOut = new BufferedOutputStream(fOut); 
     gzOut = new GzipCompressorOutputStream(bOut); 
     tOut = new TarArchiveOutputStream(gzOut); 
     addFileToTarGz(tOut, rawDir, ""); 
    } catch (FileNotFoundException e) { 
     log.error("File not found: " + e); 
    } finally { 
     if(tOut != null) { 
      tOut.finish(); 
      tOut.close(); 
     } 
    } 

Любые другие соображения или мысли по улучшению вещей?

+0

Почему, на ваш взгляд, вы хотите назвать «закончить»? – bmargulies

ответ

3

Мое понимание потоков является то, что до тех пор, как вы закрыть поток, любые потоки, инкапсулированные в него будут закрыты ...

Это правильно.

Однако ваш код (эффективно), если tOut равен null, тогда ни один из других потоков в цепочке не был создан. Это несколько изворотливое предположение. Рассмотрим эту последовательность:

  1. FileOutputStream создан и назначен fOut.
  2. Создано и создано BufferedOutputStream. bOut.
  3. Конструктор GzipCompressorOutputStream генерирует исключение или ошибку. (Может быть, куча полна ...).
  4. catch не принят ... неправильное исключение.
  5. finally проверяет tOut, находит это null и ничего не делает.

Чистый результат: мы просочились в файл дескриптор/канал, хранящийся в FileOUtputStream.

Ключ к тому, чтобы получить этот пример (абсолютно) право, состоит в том, чтобы понять, какой из этих объектов потока содержит критические ресурсы и обеспечивает закрытие потока THAT. Другие потоки, которые не содержат ресурсы, не должны быть закрыты.

} finally { 
    if (fOut != null) { 
     fOut.close(); 
    } 
} 

Другое дело, что вам нужно, чтобы переместить tOut.finish() вызов в try блока после addFileToTarGz вызова.

  • Если addFileToTarGz вызов бросает исключение, или если вы не получите, что далеко, то finish вызов является пустой тратой времени.

  • Звонок finish попытается записать индекс в архив, и это может вызвать исключение IOException. Если это произойдет в блоке finally, то любой следующий код в блоке finally, чтобы закрыть цепочку потоков, не будет выполнен ... и будет уничтожен дескриптор файла.

+1

Проверка на null добавляет сложности без причины. Инициализируйте 'fOut' при объявлении, перед' try'. – erickson

+0

@erikson - Я знаю об этом. Однако, если вы это сделаете, вам понадобится другой включенный try-catch для отчета об исключении FileNotFound. Мой ответ сосредоточен на CORRECTNESS, а не на самом элегантном решении проблемы. –

+1

Обычно, когда исключения обрабатываются на этом уровне, они обрабатываются неправильно. Поскольку метод генерирует «IOException», было бы правильным не «ловить» что-либо. Дело не в «элегантности»; проще сделать более простой код правильным. – erickson

0

Вы могли цепи все ваши конструкторы вместе, как так:

tOut = new TarArchiveOutputStream(new GzipCompressorOutputStream(new BufferedOutputStream(new FileOutputStream(archiveFile)))); 

и сэкономить 6 строк инициализации и 3 локальных переменных для отладки. Не всем нравится цепляться в этом направлении - я лично считаю его более читаемым, но остальная часть вашей команды может предпочесть его по-своему.

Что касается закрытия потока, это выглядит правильно для меня.

+3

Что произойдет, если конструктор GzipCOmpressorOutpuStream выдает исключение? как вы закрываете другие? –

+0

@GuillaumePolet - теоретически это утечка. На практике вам нужно подумать о том, что может вызвать исключение, и решить, будет ли это * вероятно * повторяться повторно. –

+0

@StephenC Согласен. Но даже если это может произойти только несколько раз, я думаю, вы должны предусмотреть эти случаи и закрыть поток в любом случае. Если никакие исключения не могут быть выбраны каким-либо из промежуточных конструкторов, конечно, это просто пустая трата времени. –

1

Хотя это выглядело бы уродливо и, может быть, вряд ли это так, вы должны закрыть их все в каскаде. Да, если вы закроете TarArchiveOutputStream, предполагается закрыть потоки underlyning. Но, в зависимости от реализации, это может быть не всегда так. Более того, и, вероятно, главным образом, если один из промежуточных конструкторов генерирует исключение, tOut будет null, но другие могут быть не такими. Это означает, что ваши потоки открыты, но вы их не закрыли.

Смежные вопросы