[New Patch] Fix disk corruption when writing

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
22 messages Options
12
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[New Patch] Fix disk corruption when writing

realitix
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

Sincerely,
Jean-Sébastien Bevilacqua

        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

Duncan Murdoch-2
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

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

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

January Weiner-3
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.

Cheers,

j.


On 4 July 2017 at 21:37, Duncan Murdoch <[hidden email]> 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
>>
>
> 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
>
>
> ______________________________________________
> [hidden email] mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel
>



--
-------- January Weiner --------------------------------------

        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

Duncan Murdoch-2
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.  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 <[hidden email]
> <mailto:[hidden email]>> 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
>
>
>     ______________________________________________
>     [hidden email] <mailto:[hidden email]> mailing list
>     https://stat.ethz.ch/mailman/listinfo/r-devel
>     <https://stat.ethz.ch/mailman/listinfo/r-devel>
>
>
>
>
> --
> -------- January Weiner --------------------------------------

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

January Weiner-3
OK, this does indeed seem to be the case. It is interesting that it works
on MacOS, though. I think that given that errors on flushing the cache
cannot be caught, the behavior is inadvertently unpredictable.

best,

j.



On 5 July 2017 at 13:09, Duncan Murdoch <[hidden email]> wrote:

> 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.  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 <[hidden email]
>> <mailto:[hidden email]>> 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
>>
>>
>>     ______________________________________________
>>     [hidden email] <mailto:[hidden email]> mailing list
>>     https://stat.ethz.ch/mailman/listinfo/r-devel
>>     <https://stat.ethz.ch/mailman/listinfo/r-devel>
>>
>>
>>
>>
>> --
>> -------- January Weiner --------------------------------------
>>
>
>


--
-------- January Weiner --------------------------------------

        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

Serguei Sokol
In reply to this post by Duncan Murdoch-2
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.

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 <[hidden email]
>> <mailto:[hidden email]>> 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
>>
>>
>>     ______________________________________________
>>     [hidden email] <mailto:[hidden email]> mailing list
>>     https://stat.ethz.ch/mailman/listinfo/r-devel
>>     <https://stat.ethz.ch/mailman/listinfo/r-devel>
>>
>>
>>
>>
>> --
>> -------- January Weiner --------------------------------------
>
> ______________________________________________
> [hidden email] mailing list
> https://stat.ethz.ch/mailman/listinfo/r-devel


--
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: [hidden email]
http://metasys.insa-toulouse.fr
http://www.lisbp.fr

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

Serguei Sokol
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 <[hidden email]
>>> <mailto:[hidden email]>> 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
>>>
>>>
>>>     ______________________________________________
>>>     [hidden email] <mailto:[hidden email]> mailing list
>>>     https://stat.ethz.ch/mailman/listinfo/r-devel
>>>     <https://stat.ethz.ch/mailman/listinfo/r-devel>
>>>
>>>
>>>
>>>
>>> --
>>> -------- January Weiner --------------------------------------
>>
>> ______________________________________________
>> [hidden email] mailing list
>> https://stat.ethz.ch/mailman/listinfo/r-devel
>
>

--
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: [hidden email]
http://metasys.insa-toulouse.fr
http://www.lisbp.fr

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

Serguei Sokol
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.

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 <[hidden email]
>>>> <mailto:[hidden email]>> 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
>>>>
>>>>
>>>>     ______________________________________________
>>>>     [hidden email] <mailto:[hidden email]> mailing list
>>>>     https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>     <https://stat.ethz.ch/mailman/listinfo/r-devel>
>>>>
>>>>
>>>>
>>>>
>>>> --
>>>> -------- January Weiner --------------------------------------
>>>
>>> ______________________________________________
>>> [hidden email] mailing list
>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>
>>
>
--
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: [hidden email]
http://metasys.insa-toulouse.fr
http://www.lisbp.fr


______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel

connection.patch (623 bytes) Download Attachment
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

Duncan Murdoch-2
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.

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.

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.

Duncan Murdoch


>
> 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 <[hidden email]
>>>>> <mailto:[hidden email]>> 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
>>>>>
>>>>>
>>>>>     ______________________________________________
>>>>>     [hidden email] <mailto:[hidden email]> mailing list
>>>>>     https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>     <https://stat.ethz.ch/mailman/listinfo/r-devel>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> -------- January Weiner --------------------------------------
>>>>
>>>> ______________________________________________
>>>> [hidden email] mailing list
>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>
>>>
>>
>

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

Serguei Sokol
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.)

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 <[hidden email]
>>>>>> <mailto:[hidden email]>> 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
>>>>>>
>>>>>>
>>>>>>     ______________________________________________
>>>>>>     [hidden email] <mailto:[hidden email]> mailing
>>>>>> list
>>>>>>     https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>> <https://stat.ethz.ch/mailman/listinfo/r-devel>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> -------- January Weiner --------------------------------------
>>>>>
>>>>> ______________________________________________
>>>>> [hidden email] mailing list
>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>
>>>>
>>>
>>
>
>


______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel

file_close.patch (397 bytes) Download Attachment
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

Duncan Murdoch-2
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.

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 <[hidden email]
>>>>>>> <mailto:[hidden email]>> 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
>>>>>>>
>>>>>>>
>>>>>>>     ______________________________________________
>>>>>>>     [hidden email] <mailto:[hidden email]> mailing
>>>>>>> list
>>>>>>>     https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>>>     <https://stat.ethz.ch/mailman/listinfo/r-devel>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> -------- January Weiner --------------------------------------
>>>>>>
>>>>>> ______________________________________________
>>>>>> [hidden email] mailing list
>>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>
>>>>>
>>>>
>>>
>>
>>
>
>
>
>

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

Serguei Sokol
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.

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 <[hidden email]
>>>>>>>> <mailto:[hidden email]>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>     ______________________________________________
>>>>>>>>     [hidden email] <mailto:[hidden email]> mailing
>>>>>>>> list
>>>>>>>>     https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>>>> <https://stat.ethz.ch/mailman/listinfo/r-devel>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> -------- January Weiner --------------------------------------
>>>>>>>
>>>>>>> ______________________________________________
>>>>>>> [hidden email] mailing list
>>>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>>
>>
>>
>>
>>
>
>

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel

close1.patch (902 bytes) Download Attachment
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

Duncan Murdoch-2
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 <[hidden email]
>>>>>>>>> <mailto:[hidden email]>> 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
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>     ______________________________________________
>>>>>>>>>     [hidden email] <mailto:[hidden email]> mailing
>>>>>>>>> list
>>>>>>>>>     https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>>>>> <https://stat.ethz.ch/mailman/listinfo/r-devel>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> -------- January Weiner --------------------------------------
>>>>>>>>
>>>>>>>> ______________________________________________
>>>>>>>> [hidden email] mailing list
>>>>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>
>>
>

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

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

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.

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

Duncan Murdoch-2
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.
>

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

Duncan Murdoch-2
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:
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.
>>
>

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

Serguei Sokol
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:[hidden email]
http://metasys.insa-toulouse.fr
http://www.lisbp.fr

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

Duncan Murdoch-2
On 10/07/2017 4:54 AM, Serguei Sokol wrote:

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

To debug your failure to close fc, reproduce the conditions before the
warning was issued, and call showConnections().

Duncan Murdoch

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

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

Serguei Sokol
Le 10/07/2017 à 11:19, Duncan Murdoch a écrit :

> On 10/07/2017 4:54 AM, Serguei Sokol wrote:
>> 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?
>
> To debug your failure to close fc, reproduce the conditions before the warning was issued, and call showConnections().
It can help in some cases but in all.
First, to reproduce the exact condition of failure is not always possible. It could
happen after a long calculation and the environment that caused
the failure could evolve meantime. And second, having the list of
connections still does not say which one (or many) has/have failed as
we have only "set[i]" not even the connection number (which in turn
could be not the same between the first failure and a tentative to reproduce it).

Is adding con->description to the warning message problematic in any sens ?

Serguei.

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [New Patch] Fix disk corruption when writing

Duncan Murdoch-2
On 10/07/2017 5:34 AM, Serguei Sokol wrote:

> Le 10/07/2017 à 11:19, Duncan Murdoch a écrit :
>> On 10/07/2017 4:54 AM, Serguei Sokol wrote:
>>> 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?
>>
>> To debug your failure to close fc, reproduce the conditions before the warning was issued, and call showConnections().
> It can help in some cases but in all.
> First, to reproduce the exact condition of failure is not always possible. It could
> happen after a long calculation and the environment that caused
> the failure could evolve meantime. And second, having the list of
> connections still does not say which one (or many) has/have failed as
> we have only "set[i]" not even the connection number (which in turn
> could be not the same between the first failure and a tentative to reproduce it).
>
> Is adding con->description to the warning message problematic in any sens ?

Yes, we don't know if it is still valid after the connection has been
closed.  It's just a pointer, whose target is allocated when the
connection is created, and deallocated when it is closed.  Using it
after closing could lead to a seg fault.

Duncan

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
12
Loading...