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

[Ivan Gerasimov]

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
>>
>
>
>