Repository navigation
Fix 52: "EEXIST:file already exists" raises during creating java project - #61
Conversation
…project, using Promise.all will cause the racing of creating the target folder.
| const projectFile: string = path.join(basePath, projectName, ".project"); | ||
| const extensionPath: string = this.context.extensionPath; | ||
| try { | ||
| await Promise.all([ |
There was a problem hiding this comment.
Maybe ensure the directory first and put all the copy work in one Promise.all()?
There was a problem hiding this comment.
There is no perf issue copying some small files, so the only concern is about the code cleanness, await Promise.all and the array of the fs operations make the code complex.
There was a problem hiding this comment.
I agree with Summer (@SummerSun) and disagree with the proposed solution. Promise.all is well known so it does not make things harder to understand. The performance downgrade is definitely noticeable when you work with larger projects. Triggering multiple copy task and let them run in parallel will make the most of modern hardware like SSD. Running them in serial manner does not make any sense.
| fse.copy(path.join(extensionPath, "templates", ".project"), projectFile), | ||
| fse.ensureDir(path.join(basePath, projectName, "bin")), | ||
| ]); | ||
| await fse.ensureDir(path.join(basePath, projectName, "bin")); |
There was a problem hiding this comment.
These two has very different meanings. The later makes it a serial operation. I don't see a problem with Promise.all. Please keep that one.
There was a problem hiding this comment.
The EEXIST:file already exists error will be thrown and the await Promise.all will reject since the fs operations will all call mkdirs on basePath + projectName
…project, using Promise.all will throw exception since the fs operations will all call mkdirs .
…andy_error_create_project
| const projectFile: string = path.join(basePath, projectName, ".project"); | ||
| const extensionPath: string = this.context.extensionPath; | ||
| try { | ||
| await Promise.all([ |
There was a problem hiding this comment.
I agree with Summer (@SummerSun) and disagree with the proposed solution. Promise.all is well known so it does not make things harder to understand. The performance downgrade is definitely noticeable when you work with larger projects. Triggering multiple copy task and let them run in parallel will make the most of modern hardware like SSD. Running them in serial manner does not make any sense.
| } | ||
|
|
||
| public static void main(String[] args) throws Exception { | ||
| System.out.println("Hello Java"); |
There was a problem hiding this comment.
Any change of this code snippet?
There was a problem hiding this comment.
Replacing tab with spaces
| const templateRoot: string = path.join(this.context.extensionPath, "templates"); | ||
| const projectFile: string = path.join(projectRoot, ".project"); | ||
| try { | ||
| fse.ensureDirSync(projectRoot); |
| const templateRoot: string = path.join(this.context.extensionPath, "templates"); | ||
| const projectFile: string = path.join(projectRoot, ".project"); | ||
| try { | ||
| fse.ensureDirSync(projectRoot); |
There was a problem hiding this comment.
Can we just await fse.ensureDir?
A sync operation blocks others especially when there is potential retry logic.
…project, using Promise.all will cause the racing of creating the target folder.
Fix #52