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

add Memory::ResetUserDict() for librime-lua release user_dict before sync data #901

Open
wants to merge 35 commits into
base: master
Choose a base branch
from

Conversation

shewer
Copy link
Contributor

@shewer shewer commented Jun 20, 2024

Pull request

改善 librime-lua中使用 Memory 類別時,sync user_data 異常
須要手動釋放 user_dict
hchunhui/librime-lua#335

Issue tracker

Fixes will automatically close the related issue

Fixes #

Feature

Describe feature of pull request

Unit test

  • Done

Manual test

  • Done

Code Review

  1. Unit and manual test pass
  2. GitHub Action CI pass
  3. At least one contributor reviews and votes
  4. Can be merged clean without conflicts
  5. PR will be merged by rebase upstream base

Additional Info

Signed-off-by: shewer <shewer@gmail.com>
Signed-off-by: shewer <shewer@gmail.com>
Signed-off-by: shewer <shewer@gmail.com>
Signed-off-by: shewer <shewer@gmail.com>
Copy link
Member

@ksqsf ksqsf left a comment

Choose a reason for hiding this comment

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

  1. API 名不應該叫 Reset,聽起來像是重新初始化用戶詞典。叫 Release 吧。
  2. 這個 API 似乎比較容易誤用。雖然看起來不會導致崩潰,但是會導致 Reset 之後 user_dict_ 再也不會回來。

Memory 析構後自然會釋放 user_dict,所以我覺得不應該通過增加一個容易誤用的 API ad hoc 地解決問題。

@shewer
Copy link
Contributor Author

shewer commented Jun 21, 2024

  1. API 名不應該叫 Reset,聽起來像是重新初始化用戶詞典。叫 Release 吧。
  2. 這個 API 似乎比較容易誤用。雖然看起來不會導致崩潰,但是會導致 Reset 之後 user_dict_ 再也不會回來。

Memory 析構後自然會釋放 user_dict,所以我覺得不應該通過增加一個容易誤用的 API ad hoc 地解決問題。

問題出在 lua gc 週期,當觸發sync時
掛在 lua 中的an 並沒有馬上釋放,
導致 user_dict sync 異常(應該是 data_sync 時 leveldb 無法開啓)

image

Signed-off-by: shewer <shewer@gmail.com>
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