From murdoch.duncan at gmail.com  Fri Jul  7 16:52:43 2017
From: murdoch.duncan at gmail.com (Duncan Murdoch)
Date: Fri, 7 Jul 2017 10:52:43 -0400
Subject: [Rd] [New Patch] Fix disk corruption when writing
In-Reply-To: <e1cf1c31-2e7c-72cb-453f-68df8321cc3f@insa-toulouse.fr>
References: <CAPXDfF=770SgtaLoMeaF7XGUfj3wFSU6hUn6ZFrds0VnwG_+zA@mail.gmail.com>
 <ba1bb252-8524-ce6d-b0d7-0d66a09789d2@gmail.com>
 <CA+A1kV5xB3yfD2SnKenrnrfgV6wDObQW8vFMxUVQd2kxTc0BjQ@mail.gmail.com>
 <1093380f-a632-ce28-af21-a87e1a40a8db@gmail.com>
 <8ede4416-7745-e66c-7d2f-53c04f1a9067@insa-toulouse.fr>
 <fe2b1e03-2f57-820e-6226-217be4184907@insa-toulouse.fr>
 <0783f593-8624-6f3f-7c54-eb3ab2938a48@insa-toulouse.fr>
 <42077a0f-0e88-8400-8310-afaaa5ef6f9e@gmail.com>
 <ed2899b8-f945-1dad-348a-cee0f8614ba7@insa-toulouse.fr>
 <e8804ff7-473f-3b89-fa65-c1b0d79d85e3@gmail.com>
 <e1cf1c31-2e7c-72cb-453f-68df8321cc3f@insa-toulouse.fr>
Message-ID: <8ca82104-19ae-a7be-d27f-9f2982b63140@gmail.com>

On 07/07/2017 9:54 AM, Serguei Sokol wrote:
> Le 07/07/2017 ? 01:09, Duncan Murdoch a ?crit :
>> On 06/07/2017 6:44 PM, Sokol Serguei wrote:
>>> Duncan Murdoch has written at  Thu, 6 Jul 2017 13:58:10 -0400
>>>> On 06/07/2017 5:21 AM, Serguei Sokol wrote:
>>>>> I propose the following patch against the current
>>>>> R-devel/src/main/connection.c (cf. attached file).
>>>>> It gives (on my linux box):
>>>>>  > fc=file("/dev/full", "w")
>>>>>  > write.csv("a", file=fc)
>>>>> Error in writeLines(paste(col.names, collapse = sep), file, sep = eol) :
>>>>>    system call failure on writing
>>>>>  > close(fc)
>>>>>
>>>>> Serguei.
>>>>
>>>> I suspect that flushing on every write will slow things down too much.
>>> That's quite plausible.
>>>
>>>>
>>>> I think a better approach is to catch the error in the Rconn_printf
>>>> calls (as R-devel currently does), and also catch errors on
>>>> con->close(con).  This one requires more changes to the source, so it
>>>> may be a day or two before I commit.
>>> I have testes it on file_close() and it works (cf. attached patch):
>>>> fc=file("/dev/full", "w")
>>>> write.csv("a", file=fc)
>>>> close(fc)
>>> Error in close.connection(fc) : closing file failed
>>>
>>>>
>>>> One thing I have to look into:  is anyone making use of the fact that
>>>> the R-level close.connection() function can return -1 to signal an
>>>> error?  Base R ignores that, which is why we need to do something, but
>>>> if packages are using it, things need to be changed carefully.  I
>>>> can't just change it to raise an error instead.
>>> As you can see in the patch, no need to change close.connection() function
>>> but we have to add a test of con->status to all *_close() functions
>>> (gzfile_close() and co.)
>>
>> You missed my point.  Currently the R close() function may return -1 to signal that there was an error closing.  We can't change that to an error if packages
>> are using it.
> May be I missed it but finally, me too, I was saying that we don't have to do so.
> Anyhow, the situation of writing to full disk cannot be passed in silence.
> IMHO, trigger an error would be the most appropriate in this situation but if for legacy
> or any other reason we cannot do so, let whistle a warning, at least.
> Here few tests with a new small patch:
>  > fc=file("/dev/full", "w"); write.csv("a", file=fc); (res=close(fc))
> [1] -1
> Warning message:
> In close.connection(fc) :
>    closing '/dev/full' failed: No space left on device
>  > fc=gzfile("/dev/full", "w"); write.csv("a", file=fc); (res=close(fc))
> NULL
> Warning message:
> In close.connection(fc) :
>    closing '/dev/full' failed: No space left on device
>  > fc=xzfile("/dev/full", "w"); write.csv("a", file=fc); (res=close(fc))
> NULL
> Warning message:
> In close.connection(fc) :
>    closing '/dev/full' failed: No space left on device
>  > fc=bzfile("/dev/full", "w"); write.csv("a", file=fc); (res=close(fc))
> NULL
> Warning message:
> In close.connection(fc) :
>    closing '/dev/full' failed: No space left on device
>
> Note that if we test only status < 0 (without errno) then there are too many warnings
> on seemingly "innocent" file closings.

Could you give an example of how to get status < 0 on a valid closing?

Duncan Murdoch

>
> Serguei.
>
>>>>>
>>>>> Le 05/07/2017 ? 15:33, Serguei Sokol a ?crit :
>>>>>> Le 05/07/2017 ? 14:46, Serguei Sokol a ?crit :
>>>>>>> Le 05/07/2017 ? 13:09, Duncan Murdoch a ?crit :
>>>>>>>> On 05/07/2017 5:26 AM, January W. wrote:
>>>>>>>>> I tried the newest patch, but it does not seem to work for me (on
>>>>>>>>> Linux). Despite the check in Rconn_printf, the write.csv happily
>>>>>>>>> writes
>>>>>>>>> to /dev/full and does not report an error. When I added a
>>>>>>>>> printf("%d\n",
>>>>>>>>> res); to the Rconn_printf() definition, I see only positive values
>>>>>>>>> returned by the vfprintf call.
>>>>>>>>>
>>>>>>>>
>>>>>>>> That's likely because you aren't writing enough to actually
>>>>>>>> trigger a write to disk during the write.  Writes are buffered,
>>>>>>>> and the error doesn't happen
>>>>>>>> until the buffer is written.
>>>>>>> I can confirm this behavior with fvprintf(). Small and medium sized
>>>>>>> writings
>>>>>>> on /dev/full don't trigger error and 1MB does.
>>>>>>>
>>>>>>> But if fprintf() is used, it returns a negative value from the very
>>>>>>> first byte written.
>>>>>> I correct myself. In my test, fprintf() returned -1 for another
>>>>>> reason (connection was already closed
>>>>>> at this moment)
>>>>>> However, if fvprintf(...) is followed by res=fflush(con) then res is -1
>>>>>> if we try to write on /dev/full. May be we have to use this to trigger
>>>>>> an error message in R.
>>>>>>
>>>>>> Serguei.
>>>>>>
>>>>>>>>   The regression test I put in had this problem; I'm working on
>>>>>>>> MacOS and Windows, so I never got to actually try it before
>>>>>>>> committing.
>>>>>>>>
>>>>>>>> Unfortunately, it doesn't look possible to catch the final flush
>>>>>>>> of the buffer when the connection is closed, so small writes won't
>>>>>>>> trigger any error.
>>>>>>>>
>>>>>>>> It's also possible that whatever system you're on doesn't signal
>>>>>>>> an error when the write fails.
>>>>>>>>
>>>>>>>> Duncan Murdoch
>>>>>>>>
>>>>>>>>> Cheers,
>>>>>>>>>
>>>>>>>>> j.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 4 July 2017 at 21:37, Duncan Murdoch <murdoch.duncan at gmail.com
>>>>>>>>> <mailto:murdoch.duncan at gmail.com>> wrote:
>>>>>>>>>
>>>>>>>>>     On 04/07/2017 11:50 AM, Jean-S?bastien Bevilacqua wrote:
>>>>>>>>>
>>>>>>>>>         Hello,
>>>>>>>>>         You can find here a patch to fix disk corruption.
>>>>>>>>>         When your disk is full, the write function exit without
>>>>>>>>> error
>>>>>>>>>         but the file
>>>>>>>>>         is truncated.
>>>>>>>>>
>>>>>>>>> https://bugs.r-project.org/bugzilla/show_bug.cgi?id=17243
>>>>>>>>> <https://bugs.r-project.org/bugzilla/show_bug.cgi?id=17243>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>     Thanks.  I didn't see that when it came through (or did and
>>>>>>>>> forgot).
>>>>>>>>>     I'll probably move the error check to a lower level (in the
>>>>>>>>>     Rconn_printf function), if tests show that works.
>>>>>>>>>
>>>>>>>>>     Duncan Murdoch
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>     ______________________________________________
>>>>>>>>>     R-devel at r-project.org <mailto:R-devel at r-project.org> mailing
>>>>>>>>> list
>>>>>>>>>     https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>>>>> <https://stat.ethz.ch/mailman/listinfo/r-devel>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> -------- January Weiner --------------------------------------
>>>>>>>>
>>>>>>>> ______________________________________________
>>>>>>>> R-devel at r-project.org mailing list
>>>>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
>



[Rd] [New Patch] Fix disk corruption when writing
Duncan Murdoch