fix: delete for _rmdirRecursiveSync #63

voltrevo · 2015-08-10T07:08:09Z

Fixes #62

silkentrance · 2015-08-12T18:26:01Z

@voltrevo Could you please add a test case for this as well?

raszi · 2015-08-14T22:07:16Z

lib/tmp.js

        dirs.push(file);
      } else {
        fs.unlinkSync(file);
-      }
-    }
+      }  


Please remove the unnecessary whitespace.

emilbayes · 2015-08-27T10:19:16Z

Need any help here? Just hit the same bug

AaronJan · 2015-09-24T09:25:55Z

Is this PR still using or not?

I got this problem today and I also made a fix( a little different ), than I saw this PR.

Need this bug fix ASAP, pretty critical.

Duplicate PR as raszi#63 , but that PR is frozen. This is a very critical matter, so I made this.

voltrevo · 2015-09-24T11:26:32Z

@AaronJan Working on this now.

voltrevo · 2015-09-24T13:18:51Z

@silkentrance @raszi I'm having a lot of trouble with the way testing works for this project:

Putting a console.log message in lib/tmp.js causes a bunch of tests to fail without real explanations
npm test relies on the previous test cleaning up after itself successfully, so when that doesn't happen the next npm test will fail even if it's done from a clean clone
there's a bunch of node sub-processes being spawned and when you console.log in them it doesn't come through to the actual terminal. Using node-debug also fails because it's debugging the process it launched and can't see these subprocesses. I've been resorting to using fs.writeFileSync to output things I need to see

Am I going about using this setup the wrong way? How do you guys debug this stuff?

voltrevo · 2015-09-24T13:26:51Z

A little more context for the above comment: I've put some code into unsafe-sync.js to setup the structure I used to trigger the error in #62:

    if (unsafe) {
      // structure from issue 62
      ['foo', 'bar'].forEach(function(dir) {
        fs.mkdirSync(join(result.name, dir));
        fs.writeFileSync(join(result.name, dir, 'baz.txt'), '');
      });
    }

Everything was good when I added that code in my branch, but I was scratching my head when all the tests passed when I restored the version of rmdirRecursiveSync with the bug in it and the tests still passed. However, by wrapping rmdirRecursiveSync in a try catch block and doing a fs.writeFileSync on an error (and re-throwing the error) I was able to get this output:

$ cat /tmp/rmdirRecError 
ENOENT: no such file or directory, scandir '/var/folders/3k/pxwb05p5219fm860z0g24jg80000gn/T/tmp-9717xrtb4pyzHICI/bar'
Error: ENOENT: no such file or directory, scandir '/var/folders/3k/pxwb05p5219fm860z0g24jg80000gn/T/tmp-9717xrtb4pyzHICI/bar'
    at Error (native)
    at Object.fs.readdirSync (fs.js:813:18)
    at _rmdirRecursiveSync (/Users/andrew/workspaces/node-tmp-issue-62/node-tmp/lib/tmp.js:270:20)
    at _cleanupCallback (/Users/andrew/workspaces/node-tmp-issue-62/node-tmp/lib/tmp.js:415:5)
    at _garbageCollector (/Users/andrew/workspaces/node-tmp-issue-62/node-tmp/lib/tmp.js:431:25)
    at process._exit (/Users/andrew/workspaces/node-tmp-issue-62/node-tmp/lib/tmp.js:457:3)
    at emitOne (events.js:77:13)
    at process.emit (events.js:169:7)
    at process.exit (node.js:737:17)
    at module.exports.exit (/Users/andrew/workspaces/node-tmp-issue-62/node-tmp/test/spawn-sync.js:25:11)

This means that errors thrown inside the node sub-processes are not propagating into the main test process. There could be other lurking issues that are being swallowed this way.

voltrevo · 2015-09-24T14:15:18Z

There we go. I added an assertion for the removal of the issue62 dir but it's actually not getting hit by the old lib/tmp.js. It looks like it actually manages to clean up that structure but throws an exception in the process which causes the main temp dir to not get cleaned up.

$ cp ../node-tmp-clean/lib/tmp.js lib/tmp.js
$ npm test

> tmp@0.0.27 test /Users/andrew/workspaces/node-tmp-issue-62/node-tmp
> vows test/*-test.js

············································✗·· ······················································ ········································ ················································ ···············  


    unsafeCleanup === true 
      ✗ should not exist 
        » Directory should be removed // dir-sync-test.js:175 
  ✗ Broken » 203 honored ∙ 1 broken (1.172s) 
npm ERR! Test failed.  See above for more details.
Andrews-MacBook-Pro:node-tmp andrew$ git checkout lib/tmp.js 
Andrews-MacBook-Pro:node-tmp andrew$ npm test

> tmp@0.0.27 test /Users/andrew/workspaces/node-tmp-issue-62/node-tmp
> vows test/*-test.js

··············································· ······················································ ········································ ················································ ···············  
  ✓ OK » 204 honored (1.165s)

AaronJan · 2015-09-24T17:14:42Z

Awesome, cheers 👍.

safanaj · 2015-09-24T20:03:03Z

👍

leahciMic · 2015-09-25T05:44:17Z

👍 i also have this issue LGTM!

silkentrance · 2015-09-25T20:32:38Z

@voltrevo as far as I understand the test cases that spawn external processes is that the output on stdout is recorded and passed to calling test case´s callback. The output is then evaluated by the test case. If you now add additional content to that output via console.log, these test cases will naturally fail.

as for debugging the external processes: http://stackoverflow.com/questions/16840623/how-to-debug-node-js-child-forked-process

silkentrance · 2015-09-25T20:46:40Z

@voltrevo and of course, spawning the child processes is required for tmp to automatically clean up after that the process exits.

silkentrance · 2015-09-25T21:29:15Z

@voltrevo in your test case you create a new assertion in dir-test which tests the asynchronous interface and you also add additional code to unsafe-sync, which is meant for the synchronous interface. as a hint: dir-test.js and unsafe.js are used in testing the asynchronous part of the api and dir-sync-test.js and unsafe-sync.js are used in testing the synchronous part of the api.

for starters, create two separate test cases, one in dir-sync-test and one in dir-test, that will each spawn a child process using the available functions in test/base.js that will then create a tmp dir either synchronously or asynchronously. In the child process you will then create a tree structure of sub directories and files just like you did in the test cases that you hooked your tests into. the child process would then console.log(path) the path to the temporary directory and simply exits.

in the test cases you would then receive the path via your provided callback and then will assert that that path, i.e. the tmp directory, no longer exists. of course you need to invoke it so that it will forward any error occurring in the child processes to your test cases.

would this be of any help to you?

voltrevo · 2015-09-26T03:44:23Z

@silkentrance Hmmm ok I'll have another crack at this.

voltrevo · 2015-09-26T04:07:46Z

@silkentrance @raszi Would you prefer the issue62 dir to be added to the structures made in unsafe.js and unsafe-sync.js and those tests added to the unsafeCleanup === true topics? Or should there be an issue62.js, issue62-sync.js, and the tests added to new topics?

I think right now I'm going to just use the existing topics, since it appears that's more consistent with the rest of the project, but if you get a chance please interrupt me if you'd like me to make separate topics.

voltrevo · 2015-09-26T04:21:52Z

@silkentrance On second thought, it definitely sounds like you want separate topics. Switching to that.

voltrevo · 2015-09-26T05:29:17Z

Ok this should be much better now, there are actually additional tests now that fail on master:

# master version is setup correctly, passes its own tests:

$ cd ../node-tmp-clean/
$ npm test

> tmp@0.0.27 test /Users/andrew/workspaces/node-tmp-issue-62/node-tmp-clean
> vows test/*-test.js

··············································· ····················································· ········································ ················································ ···············  
  ✓ OK » 203 honored (1.171s)

# using the master copy of lib/tmp.js in the new version fails the added tests:

$ cd ../node-tmp
$ cp ../node-tmp-clean/lib/tmp.js lib/tmp.js 
$ npm test

> tmp@0.0.27 test /Users/andrew/workspaces/node-tmp-issue-62/node-tmp
> vows test/*-test.js

·················································✗ ····················································✗··· ········································ ················································ ···············  


    unsafeCleanup === true with issue62 structure 
      ✗ should not exist 
        » Directory should be removed // dir-sync-test.js:199 

      ✗ should not exist 
        » Directory should be removed // dir-test.js:193 
  ✗ Broken » 207 honored ∙ 2 broken (1.254s) 
npm ERR! Test failed.  See above for more details.

# Using the new version of lib/tmp.js passes the added tests:

$ git checkout lib/tmp.js 
$ npm test

> tmp@0.0.27 test /Users/andrew/workspaces/node-tmp-issue-62/node-tmp
> vows test/*-test.js

·················································· ························································ ········································ ················································ ···············  
  ✓ OK » 209 honored (1.197s)

voltrevo · 2015-09-26T05:35:52Z

@raszi @silkentrance I'm a bit too invested to let this change go now if there are more problems with it, so I'll definitely address them if there's anything else you need me to fix up. If you like you can reach me via facebook, which you can find via an icon on the website I've listed on github. I'm much more likely to respond on there because I don't have github notifications setup very well, plus if we get a chance to chat semi-synchronously we could sort it out much faster.

Also, would you guys be interested in me doing a bit of refactoring of these tests? It would be great to have linting set up because I burned more time than I should have on an undefined reference error which wasn't propagating properly to the test output, and there was a big delay in orienting myself in the test directory because it's not really structured and there's a lot of duplicated code that could be tidied up. Can't make any promises but much more likely to submit a pr if you guys would be receptive to it.

silkentrance · 2015-09-26T14:49:58Z

@voltrevo thanks for your effort! just extend upon the existing test cases as it belongs there and is part of the standard functionality that tmp.dirSync() and tmp.dir() have to offer.

silkentrance · 2015-09-26T14:55:12Z

@voltrevo as for facebook, I am not even a member and never will be... as for the refactoring: would be fine with me, however, since I am not the one with commit rights on this repo, you will have to ask raszi 😁

voltrevo · 2015-09-27T00:05:21Z

@silkentrance I had some trouble triggering the bug using the existing test cases. In the original test I provided it managed to clean up the structure I added but caused it to fail to clean up the tmp dir itself. I think it's much better if the tests I add the ones that fail under the master version, rather than have the new test interfere with an existing test and cause it to fail.

If you insist I can look into why that happened and if there is a way to trigger just the added tests within those topics, but imo it is cleaner to do it this way anyway.

voltrevo · 2015-09-27T00:09:31Z

@silkentrance Also it sounds like maybe you just want the tests to remain in dir-test.js and dir-sync-test.js? If that's the case it's already like that. In my comment above I interpreted what you said as moving the new tests into the pre-existing topics.

raszi · 2015-09-27T20:24:11Z

Sorry for the late response. Unfortunately I don't have that much time working on this.

Thank you very much for the fix and especially to write tests for them. Probably the issue you've faced was because one of the tests failed to clean after itself which I fixed recently in 1345a23

I am aware that the tests need some refactoring and I've already started that with a grunt integration which would include linting and running the tests.
Sadly I could not have a working vows extension for grunt and I also noticed that vows is not the most used test framework among nodesters so this could be the right time to switch to something else.

raszi · 2015-09-27T20:26:06Z

So for now, I'll merge this as it is since it fixes an outstanding and we'll need to refactor the tests anyways.

Fix issue 62; duplicate delete for _rmdirRecursiveSync

voltrevo · 2015-09-28T02:00:53Z

Thanks @raszi 👍.

Fix issue 62; duplicate delete for _rmdirRecursiveSync

fbd6080

raszi reviewed Aug 14, 2015
View reviewed changes

lib/tmp.js

dirs.push(file);

} else {

fs.unlinkSync(file);

}

}

}

Copy link

Owner

raszi Aug 14, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the unnecessary whitespace.

AaronJan added a commit to AaronJan/node-tmp that referenced this pull request Sep 24, 2015

fix duplicate delete for _rmdirRecursiveSync

ed5e8c1

Duplicate PR as raszi#63 , but that PR is frozen. This is a very critical matter, so I made this.

AaronJan mentioned this pull request Sep 24, 2015

fix duplicate delete for _rmdirRecursiveSync #65

Closed

voltrevo added 2 commits September 24, 2015 21:22

Merge remote-tracking branch 'upstream/master'

3800488

Remove trailing whitespace

ce467b0

voltrevo added 2 commits September 25, 2015 00:08

Add test for issue 62

e779c64

Remove trailing whitespace

ecf7921

Move issue62 test to dir-sync-test

1ac978b

voltrevo added 3 commits September 26, 2015 14:39

Separate topic for issue62 sync test

4592bee

Add async version of issue62 test

e93eb63

Remove unnecessary arguments to issue62(sync).js

b2102fa

raszi added a commit that referenced this pull request Sep 27, 2015

Merge pull request #63 from voltrevo/master

70ffdb6

Fix issue 62; duplicate delete for _rmdirRecursiveSync

raszi merged commit 70ffdb6 into raszi:master Sep 27, 2015

raszi mentioned this pull request Sep 27, 2015

Duplicate delete in _rmdirRecursiveSync #62

Closed

raszi changed the title ~~Fix issue 62; duplicate delete for _rmdirRecursiveSync~~ fix: delete for _rmdirRecursiveSync Aug 29, 2018

silkentrance added the bug label Apr 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: delete for _rmdirRecursiveSync #63

fix: delete for _rmdirRecursiveSync #63

voltrevo commented Aug 10, 2015 •

edited by raszi

Loading

silkentrance commented Aug 12, 2015

raszi Aug 14, 2015

emilbayes commented Aug 27, 2015

AaronJan commented Sep 24, 2015

voltrevo commented Sep 24, 2015

voltrevo commented Sep 24, 2015

voltrevo commented Sep 24, 2015

voltrevo commented Sep 24, 2015

AaronJan commented Sep 24, 2015

safanaj commented Sep 24, 2015

leahciMic commented Sep 25, 2015

silkentrance commented Sep 25, 2015

silkentrance commented Sep 25, 2015

silkentrance commented Sep 25, 2015

voltrevo commented Sep 26, 2015

voltrevo commented Sep 26, 2015

voltrevo commented Sep 26, 2015

voltrevo commented Sep 26, 2015

voltrevo commented Sep 26, 2015

silkentrance commented Sep 26, 2015

silkentrance commented Sep 26, 2015

voltrevo commented Sep 27, 2015

voltrevo commented Sep 27, 2015

raszi commented Sep 27, 2015

raszi commented Sep 27, 2015

voltrevo commented Sep 28, 2015

fix: delete for _rmdirRecursiveSync #63

fix: delete for _rmdirRecursiveSync #63

Conversation

voltrevo commented Aug 10, 2015 • edited by raszi Loading

silkentrance commented Aug 12, 2015

raszi Aug 14, 2015

Choose a reason for hiding this comment

emilbayes commented Aug 27, 2015

AaronJan commented Sep 24, 2015

voltrevo commented Sep 24, 2015

voltrevo commented Sep 24, 2015

voltrevo commented Sep 24, 2015

voltrevo commented Sep 24, 2015

AaronJan commented Sep 24, 2015

safanaj commented Sep 24, 2015

leahciMic commented Sep 25, 2015

silkentrance commented Sep 25, 2015

silkentrance commented Sep 25, 2015

silkentrance commented Sep 25, 2015

voltrevo commented Sep 26, 2015

voltrevo commented Sep 26, 2015

voltrevo commented Sep 26, 2015

voltrevo commented Sep 26, 2015

voltrevo commented Sep 26, 2015

silkentrance commented Sep 26, 2015

silkentrance commented Sep 26, 2015

voltrevo commented Sep 27, 2015

voltrevo commented Sep 27, 2015

raszi commented Sep 27, 2015

raszi commented Sep 27, 2015

voltrevo commented Sep 28, 2015

voltrevo commented Aug 10, 2015 •

edited by raszi

Loading