[Vm-dev] Proposed mod to FilePlugin>>primitiveFileAtEnd for non-regular files

[Alistair Grant]

Hi Levente,

Right, my nice detailed description is all about the symptoms and not
the actual issue.  Sorry about that.

You're also correct that a new Pharo stream (ZnBufferedReadStream) had
a bug in it, which is in the process of being fixed.

The actual issue is the value returned by #atEnd, which is returning
true as you suggested:


| outputs |

outputs := OrderedCollection new.
StandardFileStream readOnlyFileNamed: '/dev/urandom' do: [ :file |
    outputs add: (file binary; next: 8).
    outputs add: file atEnd.
    outputs add: (file binary; next: 8).
    outputs add: file atEnd.
    ].
outputs
" an OrderedCollection(
    #[80 31 245 49 20 97 163 76]
    true
    #[207 210 106 185 26 86 15 49]
    true)"


The proposed modification means that #atEnd returns the correct value
for non-regular files like the example above (false), while still
maintaining the existing behaviour for regular files and stdio
streams.

Hopefully that is clearer,
Alistair


On 26 March 2018 at 18:16, Levente Uzonyi <leves at caesar.elte.hu> wrote:
>
>>
>> FilePlugin currently splits files in to two groups: 1) Stdio streams and
>> 2) everything else.
>>
>> To test for the end of file, FilePlugin>>primitiveFileAtEnd:
>>
>> 1) Uses feof() for stdio streams.
>> 2) Compares the current position to the file size for everything else.
>>
>> This returns the expected results for regular files, but fails for
>> non-regular files, e.g.:
>>
>> (FileSystem / 'dev' / 'urandom') binaryReadStream next: 8.
>>
>> returns an empty array.
>
>
> The FileStream hierachy in Squeak has no issue with the current
> implementation, because the code will call the primitive to read from the
> file even if #atEnd returns true (which conforms to what you described as
> expected usage). E.g.
>
> StandardFileStream readOnlyFileNamed: '/dev/urandom' do: [ :file |
>         file binary; next: 8 ]
>
> will return #[192 229 133 67 128 216 51 114].
>
>>
>> On Unix, the proper way to check is to read past the end of the file and
>> then call feof() to confirm that it is in fact at the end.
>>
>> Pharo (and I assume, Squeak) has plenty of code that assumes that a file
>> position >= the file size means that the file is at the end, and as
>> stated above this generally works for regular files.
>
>
> It seems that the n+1th stream implementation that was incorporated into
> Pharo is not compatible with the rest of the stream libraries (the original
> Stream hierarchy - as shown above, Nile, Xtreams, and whatever else is out
> there I'm not aware of). If it were, then this issue would have arisen long
> before.
> So, if I were you, I would first check if the new implementation in Pharo is
> correct.
>
> Levente
>
>
>>
>> This patch modifies FilePlugin>>primitiveFileAtEnd to:
>>
>> a) Keep the current behaviour of using the file position test for
>> regular files.
>> b) Keep the current behaviour of using feof() for stdio streams.
>> c) Use feof() for non-regular files, e.g. /dev/urandom.
>>
>> This allows existing code to continue to function, and allows
>> non-regular files to be tested correctly.
>>
>> After applying the patch, the example code above answers the expected
>> result, e.g.:
>>
>> (FileSystem / 'dev' / 'urandom') binaryReadStream next: 8.
>> " #[179 136 227 226 28 147 197 125]"
>>
>> On Windows, as far as I can tell, all files are regular, and the
>> position test is used regularly, so no change is requried.
>>
>> https://github.com/OpenSmalltalk/opensmalltalk-vm/pull/232
>> contains the changes.  The CI builds failed, but for different reasons
>> to do with sista.
>>
>> If I don't hear any dissenting opinion in a few days I'll integrate this
>> in.
>>
>>
>> Cheers,
>> Alistair