RFR 8023173: FileOutputStream(FileDescriptor) does not respect append flag on Windows

[roger riggs]

Looks fine.  Thanks, Roger

On 10/20/2014 4:25 PM, Ivan Gerasimov wrote:
> Thank you Roger!
>
> Yes, you're right.
> Please find the updated webrev here:
> http://cr.openjdk.java.net/~igerasim/8023173/2/webrev/
>
> Sincerely yours,
> Ivan
>
>
> On 20.10.2014 17:20, roger riggs wrote:
>> Hi Ivan,
>>
>> This webrev appears removes the ability to interpose on various method
>> using byte-code injection.  For example, FileOutputStream.write(byte).
>> Do *not *replace delete the Java method calls that call native.
>> It looks like an optimization but disables some functions that allow 
>> monitoring
>> of I/O activities such as JFR.
>>
>> Thanks, Roger
>>
>>
>> On 10/20/2014 8:34 AM, Ivan Gerasimov wrote:
>>> Thank you Alan for the review!
>>>
>>> On 17.10.2014 13:00, Alan Bateman wrote:
>>>> On 06/10/2014 11:41, Ivan Gerasimov wrote:
>>>>> Hello everybody!
>>>>>
>>>>> The append mode is emulated on Windows, therefore we have to keep 
>>>>> a flag indicating that.
>>>>>
>>>>> With the current implementation, the FileDescriptor does not know 
>>>>> if the file were opened with O_APPEND, and the flag is maintained 
>>>>> at the upper level.
>>>>> This can cause inconsistency, when the FileDescriptor is reused to 
>>>>> construct a new FileOutputStream, as there is no information 
>>>>> available about the append/non-append mode.
>>>>>
>>>>> Even though the solution is quite straight-forward: moving the 
>>>>> flag from FileOutputStream,  FileDispatcherImpl and 
>>>>> FileChannelImpl to the lower level of FileDescriptor, it touches 
>>>>> 20 files across the source-code base.
>>>>>
>>>>> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8023173
>>>>> WEBREV: http://cr.openjdk.java.net/~igerasim/8023173/0/webrev/
>>>>>
>>>>> With the fix, all the io/nio tests, including the new one, pass on 
>>>>> all available platforms.
>>>>>
>>>>> Would you please help review this fix?
>>>> Martin prodded me a few times but I was busy with other things and 
>>>> only getting to this now.
>>>>
>>>> At a high level then moving the append down to FileDescriptor make 
>>>> sense but I have a few concerns.
>>>>
>>>> On Unix systems then it looks like getAppend is calling into the 
>>>> ioctl to get the file descriptor flags.
>>>> If I read it correctly then it creates several problems for the 
>>>> usage in FileChannelImpl.position.
>>>
>>> I thought it might look like a data duplication, if we store the 
>>> append flag both in FileChannelImpl and FileDescriptor.
>>> Though, we surely can retrieve the append flag only once and cache it.
>>>
>>> I updated the webrev with this change.
>>>
>>>> I just wondering if you consider just leaving the append flag there 
>>>> and just retrieve the value once in the open method.
>>>>
>>>
>>>> I'm also wondering about the handleWrite implementation on Windows 
>>>> which changes to use additional JNI to retrieve the value of the 
>>>> append flag each time. We should try to avoid this (we want the 
>>>> native methods to be as simple as possible so that we can replace 
>>>> them in the future) so there may be an argument for passing that 
>>>> down as per the current implementation.
>>>>
>>>
>>> We would still need to retrieve the flag from native code.
>>> For example, the platform specific handleWrite() is called from the 
>>> native FileOutputStream.writeBytes(), which is called from 
>>> FileOutputStream.writt(), which is common for all the platforms.
>>> Thus, we should either retrieve the append flag from the Java code 
>>> for any platform, and ignore it later on Unix, or read the flag in 
>>> the native Windows-only code.
>>>
>>> Alternatively, we could separate implementations of 
>>> FileOutputStream.java for different platforms, but that would 
>>> complicate everything.
>>>
>>> Hopefully, in the future we could find a way to use 
>>> FILE_APPEND_DATA, so FileDescriptor.append can be removed altogether 
>>> with that corresponding JNI code.
>>>
>>> Here's the updated webrev:
>>> http://cr.openjdk.java.net/~igerasim/8023173/1/webrev/
>>>
>>> I've also included a few typo fixes left after JDK-8059840.
>>>
>>> Sincerely yours,
>>> Ivan
>>>
>>
>>
>>
>