RFR: [test,16] JDK-8164597 : TestIOException.java fails after push for JDK-8164130.
Jonathan Gibbons
jonathan.gibbons at oracle.com
Thu Jun 25 20:08:53 UTC 2020
On 6/25/20 12:16 PM, Pavel Rappo wrote:
>
>> On 25 Jun 2020, at 19:49, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
>>
>> Pavel,
>>
>> Thanks for your reasoned explanation.
>>
>> My main objection to this thread is that the code is fairly recent code (2016) that was in 8164130: Simplify doclet IOEException handling and reviewed by both Kumar and Bhavesh.
>>
>> In other words, the code works, is not broken, and it's not worth the effort to rewrite it again just to try and shrink the code by a few lines.
>>
>> In terms of general philosophy, when any IO problem occurs, I think it is important to specify the file involved, to give the end-user the best information for that person to diagnose the external condition. To me, that is better and more specific than saying "error copying from A to B, something went wrong with one of them".
>>
>> I'm also old-school enough to want to copy files of unknown size by copying the contents of a stream, rather than internalizing the contents, just to simplify the copying code.
> Understood.
>
>> I also do not like solely relying on detail messages in exceptions, and IO exceptions in particular. The messages are not localized, and there is no specification of the content. It's not even specified whether you get a meaningful subtype of IOException, like FileNotFoundException. Yes, I believe in showing the message to the user in case it might provide information, but no, I don't think we should expect anything helpful to be provided there.
> Just to comment on that bit.
>
> 1. Files.copy goes to great lengths actually specifying all subclasses of IOException it can throw. For example, it clearly says that java.nio.file.FileAlreadyExistsException will be thrown "if the target file exists but cannot be replaced because the {@code REPLACE_EXISTING} option is not specified <i>(optional specific exception)</i>".
... then tell me what exception will be thrown if the source does not
exist ;-)
>
> On a related note, that part of NIO has a nice hierarchy of exceptions, starting with "FileSystemException extends IOException".
... OK, noted, that's a step in the right direction. On the other hand,
it only has one public subtype, and is not otherwise specified to be
thrown from Files.copy.
>
> 2. Javadoc currently shows the message from the underlying IOException to the end user:
>
> } catch (DocFileIOException e) {
> switch (e.mode) {
> case READ:
> messages.error("doclet.exception.read.file",
> e.fileName.getPath(), e.getCause());
> break;
> case WRITE:
> messages.error("doclet.exception.write.file",
> e.fileName.getPath(), e.getCause());
> }
> ...
>
> doclet.exception.write.file=Error writing file: {0}\n\
> \t({1})
Yes, as I said, I believe in showing that message to the user as a
detail of last resort, after first giving the user as much definitive
information as we can provide.
>
>> In terms of Tech Debt, there are bigger fish to fry, lower-hanging fruit on the tree that deserve our attention.
> Understood.
>
>> -- Jon
>>
>>
>> On 6/25/20 11:27 AM, Pavel Rappo wrote:
>>>> On 24 Jun 2020, at 15:28, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
>>>>
>>>> Yes, it's an oversight that the ProblemList entry is commented out and not deleted. I'll fix that.
>>> Thanks.
>>>
>>>> It is intended to be helpful to users to provide details in the exception about the file causing the problem. The different exceptions identify the file in question, which is probably more important/useful than the READ/WRITE, which is there to help provide helpful diagnostic messages. This is all working around the appalling under-specification of java.io.IOException, which provides NO mandated information other than the single bit of info that "an IO error occurred".
>>> I understand the motivation you are describing and mostly agree with it. However, I think we could be helpful in a more balanced way: javadoc is likely doing more work than is required to produce quality diagnostic messages.
>>>
>>>> I don't consider it would be progress if end users have to analyze stack traces to determine the context of a message that has been reported.
>>> I do not suggest that end users should analyze stack traces; I suggest to investigate how much bigger blocks (think: Lego) of I/O functionality we can use until quality of messages that the end user sees suffers significantly. Can it be that if we accurately capture the context of an I/O operation performed by a bigger block, that context will still be enough to diagnose the problem? (That is, even if that block does not provide any diagnostic information by itself, which I don't think I have ever seen: there's always something.)
>>>
>>> For example. To copy file A to file B, javadoc does the following. It reads a portion of file A and then writes that portion to file B. This repeats until file A is exhausted and everything that has been read from file A has been also written to file B, or an error has occurred. This way, if an I/O error occurs, javadoc always knows whether this was a "read from A" or "write to B" kind of error.
>>>
>>> I think that knowing that is superfluous in most cases. If we choose a bigger block (e.g. java.nio.file.Files#copy(java.nio.file.Path, java.nio.file.Path, java.nio.file.CopyOption...)) and capture the implicated files in a DocFileIOException, that could be more practical.
>>>
>>> Could a message like "Error copying: A to B" be just as good as "Error writing file: B"? In practise, however, that difference will be more like between
>>>
>>> Error writing file: B
>>> (java.nio.file.AccessDeniedException: B)
>>>
>>> and
>>>
>>> Error copying: A to B
>>> (java.nio.file.NoSuchFileException: A)
>>> Error copying: A to B
>>> (java.nio.file.FileAlreadyExistsException: B)
>>> Error copying: A to B
>>> (java.nio.file.FileSystemException: B: Operation not permitted)
>>>
>>> -Pavel
>>>
>>>> -- Jon
>>>>
>>>> On 6/24/20 5:19 AM, Pavel Rappo wrote:
>>>>> Jon,
>>>>>
>>>>> Consider deleting that entry in ProblemList.txt instead of commenting it out. Otherwise, the change looks good.
>>>>>
>>>>> ***
>>>>>
>>>>> That change reminded me of a question I had: do we really need to split I/O errors into "read" and "write" errors and associate filenames with I/O errors on the Java level?
>>>>>
>>>>> I'm asking this because it's very tempting to refactor that code. For example, thanks to NIO, we could fold code like this into a one-liner:
>>>>>
>>>>> public void copyFile(DocFile fromFile) throws DocFileIOException {
>>>>> try (OutputStream output = openOutputStream()) {
>>>>> try (InputStream input = fromFile.openInputStream()) {
>>>>> byte[] bytearr = new byte[1024];
>>>>> int len;
>>>>> while ((len = read(fromFile, input, bytearr)) != -1) {
>>>>> write(this, output, bytearr, len);
>>>>> }
>>>>> } catch (IOException e) {
>>>>> throw new DocFileIOException(fromFile, DocFileIOException.Mode.READ, e);
>>>>> }
>>>>> } catch (IOException e) {
>>>>> throw new DocFileIOException(this, DocFileIOException.Mode.WRITE, e);
>>>>> }
>>>>> }
>>>>>
>>>>> I'm pretty sure that if any of the java.nio.file.Files's methods throws IOException, the user will be able to quickly and accurately diagnose the error using the accompanying message.
>>>>>
>>>>> Should I create an RFE to investigate that?
>>>>>
>>>>> -Pavel
>>>>>
>>>>>> On 15 Jun 2020, at 19:41, Jonathan Gibbons <jonathan.gibbons at oracle.com> wrote:
>>>>>>
>>>>>> Please review a change to enable a javadoc test to be removed from the problem list.
>>>>>>
>>>>>> The test was excluded because an IOException was occurring when running the test
>>>>>> on Windows, Most of the change here is support code to diagnose the problem, and
>>>>>> to investigate whether the issue can be fixed.
>>>>>>
>>>>>> Eventually, a reference was found on a Microsoft site, indicating that normal directories
>>>>>> cannot be made read-only on Windows. Thus the primary fix is to skip each test-case
>>>>>> if the setup for the test case fails to create a readonly directory on Windows.
>>>>>>
>>>>>> In addition doc comments and -Xdoclint:-missing are added to reduce the number of
>>>>>> irrelevant diagnostics in the output.
>>>>>>
>>>>>> In conjunction with the fix for JDK-8152313, this will fix all javadoc tests on the
>>>>>> LangTools exclude list.
>>>>>>
>>>>>> -- Jon
>>>>>>
>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8164597
>>>>>> Webrev: http://cr.openjdk.java.net/~jjg/8164597/webrev.00/
>>>>>>
More information about the javadoc-dev
mailing list