Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: adjust defer remove #237

Closed
wants to merge 1 commit into from
Closed

refactor: adjust defer remove #237

wants to merge 1 commit into from

Conversation

stevenlele
Copy link
Contributor

改正 defer 顺序,删除不需要的 defer

@moonD4rk
Copy link
Owner

moonD4rk commented Jan 9, 2024

The current method is that Golang defer follows the FIFO rule. The following code closes the DB link first, and then deletes the file after the link is closed, which is more in line with Gopher specifications.

func (f *FirefoxBookmark) Parse(_ []byte) error {
    db, err := sql.Open("sqlite3", item.TempFirefoxBookmark)
    if err != nil {
        return err
    }
    defer os.Remove(item.TempFirefoxBookmark)
    defer db.Close()
    // ......
}

Therefore, this PR will not be merged. Thank you for submitting it.

@moonD4rk moonD4rk closed this Jan 9, 2024
@stevenlele
Copy link
Contributor Author

这个代码的问题难道不是,如果 sql.Open() 失败,item.TempFirefoxBookmark 就不会被删除而是残留下来了吗?

我只是把 defer os.Remove(item.TempFirefoxBookmark) 挪到了最开头,没有调整 defer 的相对顺序。

@stevenlele
Copy link
Contributor Author

@moonD4rk ⬆️

@moonD4rk
Copy link
Owner

这个代码的问题难道不是,如果 sql.Open() 失败,item.TempFirefoxBookmark 就不会被删除而是残留下来了吗?

我只是把 defer os.Remove(item.TempFirefoxBookmark) 挪到了最开头,没有调整 defer 的相对顺序。

@stevenlele Yes, you are right. There was a bug in the previous code where temporary files could not be deleted if the program unexpectedly exited.

However, there were two considerations for doing so:

  1. An error handler was added to ensure that the program cannot exit when copying/opening files fails. fix: Improve error handling in browsing data and file copying functions #261
  2. In the next major version, HackBrowserData will provide an external API interface and be used as an SDK. In this case, database connections will be closed first before deleting files to prioritize file integrity. Additionally, this writing style is more in line with Golang syntax.

Thank you for your contribution. 🚀

@moonD4rk
Copy link
Owner

There is indeed a better way, which is to use os.TempDir() to copy the file to the temporary directory, so there is no need to consider whether sql.Open() fails.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants