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

[roger riggs]

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
>