Opened 16 months ago

Last modified 5 months ago

#9058 new bug

System.IO.openTempFile does not scale

Reported by: slyfox Owned by:
Priority: normal Milestone: 7.10.1
Component: Compiler Version: 7.8.2
Keywords: Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

In search of a bug in darcs http://bugs.darcs.net/issue2364 i've notice very bad property of openTempFile: it's pattern is very predictable and has O(n2) of already created temp files.

Predictability allows very fun bugs survive in buggy programs, like:

  thread1:
    (fn, fh) <- openTempFile "." "hello"
    renameFile fn "something"
    -- some time after
    when (some_rare_buggy_condition) $
        -- oops, reused temp name, but too late, other thread killed it
        writeFileFile fn
  thread2:
    (fn, fh) <- openTempFile "." "hello"
    workWithFn fn -- nobody should touch it, right?

It's _very_ hard to debug data corruption when all temp files are named "foo${pid}" and sometimes "foo${pid+1}".

And more serious bug: the more threads you have trying to create similar temps performance drops significantly:

Attached program shows the following numbers:

$ time ./bench-temps same 2000

real    0m2.795s
user    0m1.516s
sys     0m1.190s

$ time ./bench-temps diff 2000

real    0m0.161s
user    0m0.043s
sys     0m0.115s

It's O(N2) growing open() storm.

https://github.com/ghc/ghc/blob/master/libraries/base/System/IO.hs#L465

    FileExists -> findTempName (x + 1)

This is the source of the problem. I'd suggest always using random name for it. For portability reasons I suggest adding at least insecure random rand() value from C library.

That way we will succeed in opening temp file at the first attempt.

Attachments (3)

bench-temps.hs (985 bytes) - added by slyfox 16 months ago.
bench-temps.hs
base-openTempFile-random-name-untested.patch (2.2 KB) - added by slyfox 16 months ago.
base-openTempFile-random-name-untested.patch
T9058-do-openTempFile-be-more-random.patch (2.7 KB) - added by slyfox 16 months ago.
T9058-do-openTempFile-be-more-random.patch - this one is tested, survives bootstrap

Download all attachments as: .zip

Change History (8)

Changed 16 months ago by slyfox

bench-temps.hs

Changed 16 months ago by slyfox

base-openTempFile-random-name-untested.patch

comment:1 Changed 16 months ago by slyfox

Here is one simple solution. it basically changes

prefix ++ getpid() ++ seq_no ++ suffix

for

prefix ++ rand() ++ rand() ++ suffix

Technically rand() might be thread-unsafe (but not on glibc, there random_r() is used silently) but i don't think it's a troblem at all.

Otherwise we could use time-based timestamp, but picking precise time source (to use it more, than 1000 times a second) might be a problem.

Last edited 16 months ago by slyfox (previous) (diff)

Changed 16 months ago by slyfox

T9058-do-openTempFile-be-more-random.patch - this one is tested, survives bootstrap

comment:2 Changed 16 months ago by slyfox

  • Status changed from new to patch

comment:3 Changed 13 months ago by thoughtpolice

  • Milestone set to 7.10.1
  • Resolution set to fixed
  • Status changed from patch to closed

Merged, thanks!

comment:4 Changed 13 months ago by hvr

For the record, here's the commit that failed to use the proper ticket-reference syntax: f510c7cac5b2e9afe0ebde2766a671c59137f3cc

comment:5 Changed 5 months ago by rwbarton

  • Resolution fixed deleted
  • Status changed from closed to new

This does now mean that the sequence of filenames is completely fixed for a given program (since we don't seed the random number generator) which has some obvious downsides.

It would be nice to just use mkstemps, but maybe that is not portable enough? I wonder what other languages' standard libraries do here?

Note: See TracTickets for help on using tickets.