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 <murdoch.duncan at gmail.com >>>>>>>> <mailto:murdoch.duncan at gmail.com>> wrote: >>>>>>>> >>>>>>>> On 04/07/2017 11:50 AM, Jean-S?bastien Bevilacqua wrote: >>>>>>>> >>>>>>>> Hello, >>>>>>>> You can find here a patch to fix disk corruption. >>>>>>>> When your disk is full, the write function exit without >>>>>>>> error >>>>>>>> but the file >>>>>>>> is truncated. >>>>>>>> >>>>>>>> https://bugs.r-project.org/bugzilla/show_bug.cgi?id=17243 >>>>>>>> <https://bugs.r-project.org/bugzilla/show_bug.cgi?id=17243> >>>>>>>> >>>>>>>> >>>>>>>> Thanks. I didn't see that when it came through (or did and >>>>>>>> forgot). >>>>>>>> I'll probably move the error check to a lower level (in the >>>>>>>> Rconn_printf function), if tests show that works. >>>>>>>> >>>>>>>> Duncan Murdoch >>>>>>>> >>>>>>>> >>>>>>>> ______________________________________________ >>>>>>>> R-devel at r-project.org <mailto:R-devel at r-project.org> mailing >>>>>>>> list >>>>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel >>>>>>>> <https://stat.ethz.ch/mailman/listinfo/r-devel> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> -------- January Weiner -------------------------------------- >>>>>>> >>>>>>> ______________________________________________ >>>>>>> R-devel at r-project.org mailing list >>>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel >>>>>> >>>>>> >>>>> >>>> >>> >>> >> >> >> >> > >-------------- next part -------------- A non-text attachment was scrubbed... Name: close1.patch Type: text/x-patch Size: 870 bytes Desc: not available URL: <https://stat.ethz.ch/pipermail/r-devel/attachments/20170707/3e17e20e/attachment.bin>
On 07/07/2017 9:54 AM, Serguei Sokol wrote:> Le 07/07/2017 ? 01:09, Duncan Murdoch a ?crit : >> On 06/07/2017 6:44 PM, Sokol Serguei wrote: >>> Duncan Murdoch has written at Thu, 6 Jul 2017 13:58:10 -0400 >>>> On 06/07/2017 5:21 AM, Serguei Sokol wrote: >>>>> I propose the following patch against the current >>>>> R-devel/src/main/connection.c (cf. attached file). >>>>> It gives (on my linux box): >>>>> > fc=file("/dev/full", "w") >>>>> > write.csv("a", file=fc) >>>>> Error in writeLines(paste(col.names, collapse = sep), file, sep = eol) : >>>>> system call failure on writing >>>>> > close(fc) >>>>> >>>>> Serguei. >>>> >>>> I suspect that flushing on every write will slow things down too much. >>> That's quite plausible. >>> >>>> >>>> I think a better approach is to catch the error in the Rconn_printf >>>> calls (as R-devel currently does), and also catch errors on >>>> con->close(con). This one requires more changes to the source, so it >>>> may be a day or two before I commit. >>> I have testes it on file_close() and it works (cf. attached patch): >>>> fc=file("/dev/full", "w") >>>> write.csv("a", file=fc) >>>> close(fc) >>> Error in close.connection(fc) : closing file failed >>> >>>> >>>> One thing I have to look into: is anyone making use of the fact that >>>> the R-level close.connection() function can return -1 to signal an >>>> error? Base R ignores that, which is why we need to do something, but >>>> if packages are using it, things need to be changed carefully. I >>>> can't just change it to raise an error instead. >>> As you can see in the patch, no need to change close.connection() function >>> but we have to add a test of con->status to all *_close() functions >>> (gzfile_close() and co.) >> >> You missed my point. Currently the R close() function may return -1 to signal that there was an error closing. We can't change that to an error if packages >> are using it. > May be I missed it but finally, me too, I was saying that we don't have to do so. > Anyhow, the situation of writing to full disk cannot be passed in silence. > IMHO, trigger an error would be the most appropriate in this situation but if for legacy > or any other reason we cannot do so, let whistle a warning, at least. > Here few tests with a new small patch: > > fc=file("/dev/full", "w"); write.csv("a", file=fc); (res=close(fc)) > [1] -1 > Warning message: > In close.connection(fc) : > closing '/dev/full' failed: No space left on device > > fc=gzfile("/dev/full", "w"); write.csv("a", file=fc); (res=close(fc)) > NULL > Warning message: > In close.connection(fc) : > closing '/dev/full' failed: No space left on device > > fc=xzfile("/dev/full", "w"); write.csv("a", file=fc); (res=close(fc)) > NULL > Warning message: > In close.connection(fc) : > closing '/dev/full' failed: No space left on device > > fc=bzfile("/dev/full", "w"); write.csv("a", file=fc); (res=close(fc)) > NULL > Warning message: > In close.connection(fc) : > closing '/dev/full' failed: No space left on device > > Note that if we test only status < 0 (without errno) then there are too many warnings > on seemingly "innocent" file closings.Could you give an example of how to get status < 0 on a valid closing? Duncan Murdoch> > Serguei. > >>>>> >>>>> Le 05/07/2017 ? 15:33, Serguei Sokol a ?crit : >>>>>> Le 05/07/2017 ? 14:46, Serguei Sokol a ?crit : >>>>>>> Le 05/07/2017 ? 13:09, Duncan Murdoch a ?crit : >>>>>>>> On 05/07/2017 5:26 AM, January W. wrote: >>>>>>>>> I tried the newest patch, but it does not seem to work for me (on >>>>>>>>> Linux). Despite the check in Rconn_printf, the write.csv happily >>>>>>>>> writes >>>>>>>>> to /dev/full and does not report an error. When I added a >>>>>>>>> printf("%d\n", >>>>>>>>> res); to the Rconn_printf() definition, I see only positive values >>>>>>>>> returned by the vfprintf call. >>>>>>>>> >>>>>>>> >>>>>>>> That's likely because you aren't writing enough to actually >>>>>>>> trigger a write to disk during the write. Writes are buffered, >>>>>>>> and the error doesn't happen >>>>>>>> until the buffer is written. >>>>>>> I can confirm this behavior with fvprintf(). Small and medium sized >>>>>>> writings >>>>>>> on /dev/full don't trigger error and 1MB does. >>>>>>> >>>>>>> But if fprintf() is used, it returns a negative value from the very >>>>>>> first byte written. >>>>>> I correct myself. In my test, fprintf() returned -1 for another >>>>>> reason (connection was already closed >>>>>> at this moment) >>>>>> However, if fvprintf(...) is followed by res=fflush(con) then res is -1 >>>>>> if we try to write on /dev/full. May be we have to use this to trigger >>>>>> an error message in R. >>>>>> >>>>>> Serguei. >>>>>> >>>>>>>> The regression test I put in had this problem; I'm working on >>>>>>>> MacOS and Windows, so I never got to actually try it before >>>>>>>> committing. >>>>>>>> >>>>>>>> Unfortunately, it doesn't look possible to catch the final flush >>>>>>>> of the buffer when the connection is closed, so small writes won't >>>>>>>> trigger any error. >>>>>>>> >>>>>>>> It's also possible that whatever system you're on doesn't signal >>>>>>>> an error when the write fails. >>>>>>>> >>>>>>>> Duncan Murdoch >>>>>>>> >>>>>>>>> Cheers, >>>>>>>>> >>>>>>>>> j. >>>>>>>>> >>>>>>>>> >>>>>>>>> On 4 July 2017 at 21:37, Duncan Murdoch <murdoch.duncan at gmail.com >>>>>>>>> <mailto:murdoch.duncan at gmail.com>> wrote: >>>>>>>>> >>>>>>>>> On 04/07/2017 11:50 AM, Jean-S?bastien Bevilacqua wrote: >>>>>>>>> >>>>>>>>> Hello, >>>>>>>>> You can find here a patch to fix disk corruption. >>>>>>>>> When your disk is full, the write function exit without >>>>>>>>> error >>>>>>>>> but the file >>>>>>>>> is truncated. >>>>>>>>> >>>>>>>>> https://bugs.r-project.org/bugzilla/show_bug.cgi?id=17243 >>>>>>>>> <https://bugs.r-project.org/bugzilla/show_bug.cgi?id=17243> >>>>>>>>> >>>>>>>>> >>>>>>>>> Thanks. I didn't see that when it came through (or did and >>>>>>>>> forgot). >>>>>>>>> I'll probably move the error check to a lower level (in the >>>>>>>>> Rconn_printf function), if tests show that works. >>>>>>>>> >>>>>>>>> Duncan Murdoch >>>>>>>>> >>>>>>>>> >>>>>>>>> ______________________________________________ >>>>>>>>> R-devel at r-project.org <mailto:R-devel at r-project.org> mailing >>>>>>>>> list >>>>>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel >>>>>>>>> <https://stat.ethz.ch/mailman/listinfo/r-devel> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> -------- January Weiner -------------------------------------- >>>>>>>> >>>>>>>> ______________________________________________ >>>>>>>> R-devel at r-project.org mailing list >>>>>>>> https://stat.ethz.ch/mailman/listinfo/r-devel >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>>> >>> >>> >>> >>> >> >> >
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.