From sokol at insa-toulouse.fr  Mon Jul 10 10:54:34 2017
From: sokol at insa-toulouse.fr (Serguei Sokol)
Date: Mon, 10 Jul 2017 10:54:34 +0200
Subject: [Rd] [New Patch] Fix disk corruption when writing
In-Reply-To: <a5920916-a708-14d3-0e0f-66e5f35f7871@gmail.com>
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>
 <8ca82104-19ae-a7be-d27f-9f2982b63140@gmail.com>
 <ce8dbd19-c6f1-fcf0-c9af-fccc27eb37c8@insa-toulouse.fr>
 <02239f51-3592-d3f3-5f4a-f479b35b4da9@gmail.com>
 <a5920916-a708-14d3-0e0f-66e5f35f7871@gmail.com>
Message-ID: <ce5bb61d-0eb4-cc7d-49ec-d43b8de14eab@insa-toulouse.fr>

Le 08/07/2017 ? 00:54, Duncan Murdoch a ?crit :
> I have now committed changes to R-devel (rev 72898) that seem to catch large and small errors.  They only give a warning if the error happens when the 
> connection is closed, because that can happen asynchronously
For this asynchronous behavior, would not it be more useful to have
the name of the file that failed at closing? If many files were open
during a session and not closed explicitly (yes, bad practice but it
can happen), the warning message doesn't help to understand
which of files were corrupted, e.g.:
 > fc=file("/dev/full", "w")
 > write.csv("a", file=fc)
 > q("yes")
Warning message:
In close.connection(getConnection(set[i])) :
   Problem closing connection:  No space left on device

Having only "set[i]" for indication is not very informative, is it?

Serguei.

> : I didn't want to mess up some later unrelated computation that triggered garbage collection.
>
> I will wait a while before porting these to R-patched, because there may still be some problems to clean up.
>
> Duncan Murdoch
>
>
>
> On 07/07/2017 11:42 AM, Duncan Murdoch wrote:
>> On 07/07/2017 11:13 AM, Serguei Sokol wrote:
>>> Le 07/07/2017 ? 16:52, Duncan Murdoch a ?crit :
>>>> 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?
>>> If you remove "&& errno" and leave only "if (status < 0)" in the previous patch
>>> then during make I have many warnings, e.g. :
>>> Warning messages:
>>> 1: In close.connection(con) :
>>>    closing '/home/sokol/dev/R/R-devel/library/compiler/Meta/nsInfo.rds' failed: Success
>>> 2: In close.connection(con) :
>>>    closing '/home/sokol/dev/R/R-devel/library/compiler/Meta/package.rds' failed: Success
>>> 3: In close(con) :
>>>    closing '/home/sokol/dev/R/R-devel/library/compiler/R/compiler.rdx' failed: Success
>>> 4: In close.connection(con) :
>>>    closing '/home/sokol/dev/R/R-devel/library/tools/Meta/nsInfo.rds' failed: Success
>>> 5: In close.connection(con) :
>>>    closing '/home/sokol/dev/R/R-devel/library/tools/Meta/package.rds' failed: Success
>>> 6: In close(con) :
>>>    closing '/home/sokol/dev/R/R-devel/library/tools/R/tools.rdx' failed: Success
>>> 7: In close(con) :
>>>    closing '/home/sokol/dev/R/R-devel/library/tools/R/sysdata.rdx' failed: Success
>>> 8: In close.connection(con) :
>>>    closing '../../library/parallel/Meta/Rd.rds' failed: Success
>>> 9: In close.connection(con) :
>>>    closing '../../library/parallel/help/aliases.rds' failed: Success
>>> 10: In close.connection(file) :
>>>    closing '../../library/parallel/DESCRIPTION' failed: Success
>>>
>>
>> You are probably seeing cases where status is never set:  then status is
>> NA_INTEGER, which (in C) is negative.
>>
>> Duncan Murdoch
>>
>>
>>> Note "Succes" as the reason of "failure".
>>>
>>> And if I use thus compiled R, at startup I get:
>>>
>>> Warning message:
>>> In close(con) :
>>>    closing '/home/sokol/dev/R/R-devel/library/base/R/base.rdx' failed: Success
>>>
>>> R Under development (unstable) (2017-06-01 r72753) -- "Unsuffered Consequences"
>>> Copyright (C) 2017 The R Foundation for Statistical Computing
>>> Platform: x86_64-pc-linux-gnu (64-bit)
>>>
>>> R is free software and comes with ABSOLUTELY NO WARRANTY.
>>> You are welcome to redistribute it under certain conditions.
>>> Type 'license()' or 'licence()' for distribution details.
>>>
>>> R is a collaborative project with many contributors.
>>> Type 'contributors()' for more information and
>>> 'citation()' on how to cite R or R packages in publications.
>>>
>>> Type 'demo()' for some demos, 'help()' for on-line help, or
>>> 'help.start()' for an HTML browser interface to help.
>>> Type 'q()' to quit R.
>>>
>>> Warning messages:
>>> 1: In close.connection(con) :
>>>    closing '/home/sokol/dev/R/R-devel/library/methods/Meta/package.rds' failed: Success
>>> 2: In close.connection(con) :
>>>    closing '/home/sokol/dev/R/R-devel/library/methods/Meta/features.rds' failed: Success
>>> 3: In close.connection(con) :
>>>    closing '/home/sokol/dev/R/R-devel/library/methods/Meta/nsInfo.rds' failed: Success
>>> 4: In close.connection(con) :
>>>    closing '/home/sokol/dev/R/R-devel/library/methods/Meta/package.rds' failed: Success
>>> 5: In close(con) :
>>>    closing '/home/sokol/dev/R/R-devel/library/methods/R/methods.rdx' failed: Success
>>> [Previously saved workspace restored]
>>>
>>> During startup - There were 27 warnings (use warnings() to see them)
>>>
>>> All these closings seem valid to me but obviously the warnings indicate that status was < 0.
>>>
>>> Finaly, if I open and close a valid file in this session, I don't get a warning for _this_ file:
>>>  > fc=file("/tmp/aha", "w")
>>> Warning messages:
>>> 1: In close.connection(con) :
>>>    closing '/home/sokol/dev/R/R-devel/library/compiler/Meta/nsInfo.rds' failed: Success
>>> 2: In close.connection(con) :
>>>    closing '/home/sokol/dev/R/R-devel/library/compiler/Meta/package.rds' failed: Success
>>> 3: In close(con) :
>>>    closing '/home/sokol/dev/R/R-devel/library/compiler/R/compiler.rdx' failed: Success
>>>  > write.csv("a", file=fc)
>>>  > close(fc) # no warning
>>>
>>>
>>> Serguei.
>>>
>>
>
>

-- 
Serguei Sokol
Ingenieur de recherche INRA
Metabolisme Integre et Dynamique des Systemes Metaboliques (MetaSys)

LISBP, INSA/INRA UMR 792, INSA/CNRS UMR 5504
135 Avenue de Rangueil
31077 Toulouse Cedex 04

tel: +33 5 6155 9276
fax: +33 5 6704 8825
email:sokol at insa-toulouse.fr
http://metasys.insa-toulouse.fr
http://www.lisbp.fr



[Rd] [New Patch] Fix disk corruption when writing
Serguei Sokol