Atomic rename #47

develar · 2017-02-24T10:22:58Z

#36 (comment)

Already used in the electron-builder. During test period no issues were discovered.

Close #36

# Conflicts: # package.json # test/test_bad_config.js

develar · 2017-03-18T17:48:22Z

@malept Could you please review?

MarshallOfSound

RIP, not getting notifications from this repo 😆

LGTM, only issue would be breach of scope (the package.json changes, but they all look fair enough so I'm 👍 )

MarshallOfSound · 2017-03-21T08:45:00Z

Probably want a second set of eyes before merge into a key module

/cc @electron-userland

malept · 2017-03-21T02:37:13Z

package.json

    "mkdirp": "^0.5.1",
-    "tape": "^4.6.0",


The changes in this file are unnecessary.

malept · 2017-03-21T02:42:18Z

lib/index.js

-      })
+    debug('moving', filename, 'from', this.cache, 'to', target)
+    fs.rename(path.join(this.cache, filename), target, (err) => {
+      if (err) return cb(err)


So if rename fails, we're just going to leave the file in the cache dir?

This should clean up after itself.

malept · 2017-03-21T14:37:00Z

lib/index.js

-  downloadFile (url, filename, cacheFilename, cb, onSuccess) {
-    debug('downloading', url, 'to', this.tmpdir)
+  downloadFile (url, cacheFilename, cb, onSuccess) {
+    const tempFileName = `tmp-${process.pid}-${(tmpFileCounter++).toString(16)}-${path.basename(cacheFilename)}`


I'd rather have this use something like tempnam instead of introducing global variables and hand-rolling it.

tempnam is not suitable because using fs. I think it is ok to not use external module for two lines of code (also, to reduce number of external deps and increase security).

tempnam is not suitable because using fs.

Could you please explain this?

I don't want to use something like mkdtemp here because only our tool creates temp file here — process.pid allows us to be sure that will be no conflicts with other processes. To not complicate and keep it simple.

malept · 2017-03-21T15:12:59Z

lib/index.js

@@ -10,6 +10,8 @@ const pathExists = require('path-exists')
 const semver = require('semver')
 const sumchecker = require('sumchecker')

+let tmpFileCounter = 0


If you're going to keep this, at least make it a class variable or something.

It must be file level variable because class can be created several times.

Yes, a class variable, not an instance variable.

a class variable, not an instance variable.

Hmm... ES6 doesn't support

static tmpFileCounter = 0

SyntaxError: Unexpected token =

We cannot use this syntax because node doesn't support it.

What do you mean? And for what we should use class variable here? It should be module level variable. e.g. I write in Kotlin and prefer to use file level vars instead of static.

@develar That syntax isn't supported but the syntax ClassName.tmpFileCounter = 0 does work and has the same affect 👍

malept · 2017-03-21T15:13:29Z

lib/index.js

-  downloadFile (url, filename, cacheFilename, cb, onSuccess) {
-    debug('downloading', url, 'to', this.tmpdir)
+  downloadFile (url, cacheFilename, cb, onSuccess) {
+    const tempFileName = `tmp-${process.pid}-${(tmpFileCounter++).toString(16)}-${path.basename(cacheFilename)}`


tempnam is not suitable because using fs.

Could you please explain this?

malept · 2017-03-21T15:13:59Z

lib/index.js

+    fs.rename(path.join(cache, filename), target, (err) => {
+      if (err) {
+        try {
+          fs.unlinkSync(cache)


Why not use the async version and get rid of the try...catch?

malept · 2017-03-21T15:14:21Z

lib/index.js

+        try {
+          fs.unlinkSync(cache)
+        } catch (error) {
+          console.error(`Error deleting cache file: ${error.message}`)


This needs to show the filename that couldn't be removed so the user can deal with it themselves.

I don't think this was addressed.

develar · 2017-03-27T18:26:54Z

I have answered to all questions, what's left?

kevinsawicki · 2017-03-28T02:07:11Z

I have answered to all questions, what's left?

This looks good to me, what do you think @malept?

malept · 2017-03-29T07:02:51Z

lib/index.js

+            }
+          } finally {
+            cb(err)
+          }


Not sure what the point of the try...finally is. Is an exception being thrown?

☝️ Yeah it seems unnecessary...

User application can override console.error. e.g. Jest does it. As we must call user cb in any case, it is more safe to use try-finally here. Otherwise we can get unresolved promise and so on.

Jest does it

Yikes. Ok.

malept · 2017-03-29T07:03:35Z

@zeke given #36, do you want to take a look?

zeke

I agree with @malept's recent feedback all around. Otherwise, looks good to me!

develar · 2017-04-08T15:37:29Z

I don't like ElectronDownloader.tmpFileCounter (local module variable is better and more safe), but if you want — ok.

develar · 2017-04-09T18:28:54Z

Right now electron-download should be not released after #24 — I get strange errors like Error: ENOENT: no such file or directory, open '/Users/develar/Library/Caches/electron/electron-v1.6.3-darwin-x64.zip And since electron-download (and used libs) doesn't use promises with long stack traces, it is not easy to find cause of error. Not yet clear — is it due to my changes or upstream affected as well.

@Siilwyn maybe you will have some ideas (workaround — remove ~/.electron dir).

Siilwyn · 2017-04-09T19:10:29Z

@develar I'm assuming you already found the culprit pointed out in #24

develar · 2017-04-15T10:07:18Z

Friendly ping :)

kevinsawicki · 2017-04-21T15:47:34Z

Thanks @develar 👍 🚢

develar added 5 commits February 2, 2017 10:40

bump dependencies, require node 4, remove home-path dependency

32cb2af

resolve merge conflicts

cfe9c28

atomic rename, download to cache dir (VM support, avoid access denied)

feca72e

Merge remote-tracking branch 'upstream/master' into atomic-rename2

8c1005e

# Conflicts: # package.json # test/test_bad_config.js

update deps

970ee6d

develar mentioned this pull request Mar 21, 2017

Move to electron-builder iffy/electron-updater-example#8

Open

MarshallOfSound approved these changes Mar 21, 2017

View reviewed changes

malept requested changes Mar 21, 2017

View reviewed changes

delete cache file

48bab3b

malept requested changes Mar 21, 2017

View reviewed changes

use async unlink

8ef849a

malept reviewed Mar 29, 2017

View reviewed changes

zeke suggested changes Mar 29, 2017

View reviewed changes

develar added 2 commits April 1, 2017 10:23

Merge remote-tracking branch 'upstream/master' into atomic-rename2

f1e0f1b

ElectronDownloader.tmpFileCounter

f41cd82

develar mentioned this pull request Apr 9, 2017

Save cached releases to standard directories #24

Closed

Siilwyn mentioned this pull request Apr 14, 2017

Fix fallback location #54

Merged

kevinsawicki merged commit 9e43a90 into electron:master Apr 21, 2017

develar deleted the atomic-rename branch August 24, 2017 09:45

MarshallOfSound mentioned this pull request Feb 27, 2018

Permission Denied when Deploying on AWS and creating the temporary directory #39

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Atomic rename #47

Atomic rename #47

develar commented Feb 24, 2017 •

edited

Loading

develar commented Mar 18, 2017

MarshallOfSound left a comment

MarshallOfSound commented Mar 21, 2017

malept Mar 21, 2017

malept Mar 21, 2017

malept Mar 21, 2017

develar Mar 21, 2017

malept Mar 21, 2017

develar Mar 21, 2017

malept Mar 21, 2017

develar Mar 22, 2017

malept Mar 29, 2017

develar Apr 1, 2017

MarshallOfSound Apr 1, 2017

malept Mar 21, 2017

malept Mar 21, 2017

malept Mar 21, 2017

malept Mar 29, 2017

develar commented Mar 27, 2017

kevinsawicki commented Mar 28, 2017

malept Mar 29, 2017

zeke Mar 29, 2017

develar Mar 29, 2017 •

edited

Loading

zeke Mar 29, 2017

malept commented Mar 29, 2017

zeke left a comment

develar commented Apr 8, 2017

develar commented Apr 9, 2017 •

edited

Loading

Siilwyn commented Apr 9, 2017

develar commented Apr 15, 2017

kevinsawicki commented Apr 21, 2017

Atomic rename #47

Atomic rename #47

Conversation

develar commented Feb 24, 2017 • edited Loading

develar commented Mar 18, 2017

MarshallOfSound left a comment

Choose a reason for hiding this comment

MarshallOfSound commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

develar commented Mar 27, 2017

kevinsawicki commented Mar 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

develar Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malept commented Mar 29, 2017

zeke left a comment

Choose a reason for hiding this comment

develar commented Apr 8, 2017

develar commented Apr 9, 2017 • edited Loading

Siilwyn commented Apr 9, 2017

develar commented Apr 15, 2017

kevinsawicki commented Apr 21, 2017

develar commented Feb 24, 2017 •

edited

Loading

develar Mar 29, 2017 •

edited

Loading

develar commented Apr 9, 2017 •

edited

Loading