code review 145860043: os: avoid error result when dir is removed out from und...

os: avoid error result when dir is removed out from under RemoveAll

Fixes issue 7776.

[rsc, 2014-09-17 20:55:52]

Hello golang-codereviews@googlegroups.com (cc: r),

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2014-09-17 20:57:52]

LGTM


On Wed, Sep 17, 2014 at 4:55 PM, <rsc@golang.org> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com (cc: r),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> os: avoid error result when dir is removed out from under RemoveAll
>
> Fixes issue 7776.
>
> Please review this at https://codereview.appspot.com/145860043/
>
> Affected files (+46, -1 lines):
>   M src/os/os_test.go
>   M src/os/path.go
>
>
> Index: src/os/os_test.go
> ===================================================================
> --- a/src/os/os_test.go
> +++ b/src/os/os_test.go
> @@ -18,6 +18,7 @@
>         "runtime"
>         "sort"
>         "strings"
> +       "sync"
>         "syscall"
>         "testing"
>         "text/template"
> @@ -1403,3 +1404,44 @@
>                 }
>         }
>  }
> +
> +func mkdirTree(t *testing.T, root string, level, max int) {
> +       if level >= max {
> +               return
> +       }
> +       level++
> +       for i := 'a'; i < 'c'; i++ {
> +               dir := filepath.Join(root, string(i))
> +               if err := Mkdir(dir, 0700); err != nil {
> +                       t.Fatal(err)
> +               }
> +               mkdirTree(t, dir, level, max)
> +       }
> +}
> +
> +// Test that simultaneous RemoveAll do not report an error.
> +// As long as it gets removed, we should be happy.
> +func TestRemoveAllRace(t *testing.T) {
> +       n := runtime.GOMAXPROCS(16)
> +       defer runtime.GOMAXPROCS(n)
> +       root, err := ioutil.TempDir("", "issue")
> +       if err != nil {
> +               t.Fatal(err)
> +       }
> +       mkdirTree(t, root, 1, 6)
> +       hold := make(chan struct{})
> +       var wg sync.WaitGroup
> +       for i := 0; i < 4; i++ {
> +               wg.Add(1)
> +               go func() {
> +                       defer wg.Done()
> +                       <-hold
> +                       err := RemoveAll(root)
> +                       if err != nil {
> +                               t.Errorf("unexpected error: %T, %q", err,
> err)
> +                       }
> +               }()
> +       }
> +       close(hold) // let workers race to remove root
> +       wg.Wait()
> +}
> Index: src/os/path.go
> ===================================================================
> --- a/src/os/path.go
> +++ b/src/os/path.go
> @@ -66,7 +66,7 @@
>  func RemoveAll(path string) error {
>         // Simple case: if Remove works, we're done.
>         err := Remove(path)
> -       if err == nil {
> +       if err == nil || IsNotExist(err) {
>                 return nil
>         }
>
> @@ -116,6 +116,9 @@
>
>         // Remove directory.
>         err1 := Remove(path)
> +       if err1 == nil || IsNotExist(err1) {
> +               return nil
> +       }
>         if err == nil {
>                 err = err1
>         }
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[bradfitz, 2014-09-17 20:59:40]

LGTM still but can you skip that test in testing.Short() mode? It's very
fs-intensive.


On Wed, Sep 17, 2014 at 4:57 PM, Brad Fitzpatrick <bradfitz@golang.org>
wrote:

> LGTM
>
>
> On Wed, Sep 17, 2014 at 4:55 PM, <rsc@golang.org> wrote:
>
>> Reviewers: golang-codereviews,
>>
>> Message:
>> Hello golang-codereviews@googlegroups.com (cc: r),
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go/
>>
>>
>> Description:
>> os: avoid error result when dir is removed out from under RemoveAll
>>
>> Fixes issue 7776.
>>
>> Please review this at https://codereview.appspot.com/145860043/
>>
>> Affected files (+46, -1 lines):
>>   M src/os/os_test.go
>>   M src/os/path.go
>>
>>
>> Index: src/os/os_test.go
>> ===================================================================
>> --- a/src/os/os_test.go
>> +++ b/src/os/os_test.go
>> @@ -18,6 +18,7 @@
>>         "runtime"
>>         "sort"
>>         "strings"
>> +       "sync"
>>         "syscall"
>>         "testing"
>>         "text/template"
>> @@ -1403,3 +1404,44 @@
>>                 }
>>         }
>>  }
>> +
>> +func mkdirTree(t *testing.T, root string, level, max int) {
>> +       if level >= max {
>> +               return
>> +       }
>> +       level++
>> +       for i := 'a'; i < 'c'; i++ {
>> +               dir := filepath.Join(root, string(i))
>> +               if err := Mkdir(dir, 0700); err != nil {
>> +                       t.Fatal(err)
>> +               }
>> +               mkdirTree(t, dir, level, max)
>> +       }
>> +}
>> +
>> +// Test that simultaneous RemoveAll do not report an error.
>> +// As long as it gets removed, we should be happy.
>> +func TestRemoveAllRace(t *testing.T) {
>> +       n := runtime.GOMAXPROCS(16)
>> +       defer runtime.GOMAXPROCS(n)
>> +       root, err := ioutil.TempDir("", "issue")
>> +       if err != nil {
>> +               t.Fatal(err)
>> +       }
>> +       mkdirTree(t, root, 1, 6)
>> +       hold := make(chan struct{})
>> +       var wg sync.WaitGroup
>> +       for i := 0; i < 4; i++ {
>> +               wg.Add(1)
>> +               go func() {
>> +                       defer wg.Done()
>> +                       <-hold
>> +                       err := RemoveAll(root)
>> +                       if err != nil {
>> +                               t.Errorf("unexpected error: %T, %q", err,
>> err)
>> +                       }
>> +               }()
>> +       }
>> +       close(hold) // let workers race to remove root
>> +       wg.Wait()
>> +}
>> Index: src/os/path.go
>> ===================================================================
>> --- a/src/os/path.go
>> +++ b/src/os/path.go
>> @@ -66,7 +66,7 @@
>>  func RemoveAll(path string) error {
>>         // Simple case: if Remove works, we're done.
>>         err := Remove(path)
>> -       if err == nil {
>> +       if err == nil || IsNotExist(err) {
>>                 return nil
>>         }
>>
>> @@ -116,6 +116,9 @@
>>
>>         // Remove directory.
>>         err1 := Remove(path)
>> +       if err1 == nil || IsNotExist(err1) {
>> +               return nil
>> +       }
>>         if err == nil {
>>                 err = err1
>>         }
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "golang-codereviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-codereviews+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.
>>
>
>

[rsc, 2014-09-17 21:04:46]

On Wed, Sep 17, 2014 at 4:59 PM, Brad Fitzpatrick <bradfitz@golang.org>
wrote:

> LGTM still but can you skip that test in testing.Short() mode? It's very
> fs-intensive.
>

It creates 32 directories. Is that really fs-intensive?

[bradfitz, 2014-09-17 21:35:00]

On Wed, Sep 17, 2014 at 5:04 PM, Russ Cox <rsc@golang.org> wrote:

> On Wed, Sep 17, 2014 at 4:59 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
>
>> LGTM still but can you skip that test in testing.Short() mode? It's very
>> fs-intensive.
>>
>
> It creates 32 directories. Is that really fs-intensive?
>

 I don't like tests writing at all. I'm not fussed about it either way,
though. Submit away.

[rsc, 2014-09-18 18:48:50]

*** Submitted as https://code.google.com/p/go/source/detail?r=50e833e16e33 ***

os: avoid error result when dir is removed out from under RemoveAll

Fixes issue 7776.

LGTM=bradfitz
R=golang-codereviews, bradfitz
CC=golang-codereviews, r
https://codereview.appspot.com/145860043

[brainman, 2014-09-18 23:45:03]

This broke windows build:

--- FAIL: TestRemoveAllRace (0.11s)
	os_test.go:1441: unexpected error: *os.PathError, "open
C:\\Users\\WINDOW~1\\AppData\\Local\\Temp\\2\\issue611815586\\a\\b\\b\\a: Access
is denied."
	os_test.go:1441: unexpected error: *os.PathError, "remove
C:\\Users\\WINDOW~1\\AppData\\Local\\Temp\\2\\issue611815586\\a: Access is
denied."
FAIL
FAIL	os	1.783s