Skip to content

feat(cinbase): asynchronous table loading with Future and CinTableBase - #1004

Open
PCMan wants to merge 1 commit into
masterfrom
feature/cinbase-table-manager
Open

PCMan wants to merge 1 commit into
masterfrom
feature/cinbase-table-manager

Conversation

@PCMan

@PCMan PCMan commented Oct 25, 2026 •

Copy link
Copy Markdown
Member

Replace threading.Thread table loaders with a shared ThreadPoolExecutor and introduce CinTableBase to encapsulate table loading state:

  • Introduce CinTableBase in python/cinbase/cintablebase.py with Future-backed dynamic loading and cin properties, plus non-blocking wait().
  • Refactor LoadCinTable, LoadRCinTable, and LoadHCinTable in python/cinbase/__init__.py into non-blocking start() calls that delegate file IO to a ThreadPoolExecutor.
  • Update all 8 CIN-based IME modules to subclass CinTableBase and replace busy-wait sleep loops with CinTable.wait().
  • Add unit tests for CinTableBase in tests/test_cintablebase.py.

This eliminates CPU-spinning busy-wait polling loops and prevents worker threads from mutating shared frontend state.

Architecture and design by Hong Jen Yee (PCMan). Implementation generated with AI assistance from Google Antigravity (Gemini 3.8 Flash).

@PCMan
PCMan force-pushed the feature/cinbase-table-manager branch from 6cb393d to 88e1188 Compare October 25, 2026 03:52
@PCMan PCMan changed the title fix(cinbase): centralize table loading with CinTableManager and concurrent.futures feat(cinbase): add CinTableManager and unit tests Oct 25, 2026
@PCMan
PCMan force-pushed the feature/cinbase-table-manager branch from 88e1188 to f321ffb Compare October 25, 2026 03:53
@PCMan PCMan changed the title feat(cinbase): add CinTableManager and unit tests feat(cinbase): asynchronous table loading and migration to CinTableBase Oct 25, 2026
@PCMan
PCMan force-pushed the feature/cinbase-table-manager branch 3 times, most recently from e9e804b to 54d55ea Compare October 25, 2026 05:54
@PCMan PCMan changed the title feat(cinbase): asynchronous table loading and migration to CinTableBase feat(cinbase): add CinTableBase and replace thread with future Oct 25, 2026
@PCMan
PCMan force-pushed the feature/cinbase-table-manager branch 9 times, most recently from 2f9bfde to 7596e44 Compare October 25, 2026 06:49
- Introduce CinTableBase in python/cinbase/cintablebase.py with Future-backed dynamic loading and cin properties, plus non-blocking wait().
- Refactor LoadCinTable, LoadRCinTable, and LoadHCinTable in python/cinbase/__init__.py into non-blocking start() calls that delegate file IO to a ThreadPoolExecutor.
- Update all 8 CIN-based IME modules to subclass CinTableBase and replace busy-wait sleep loops with CinTable.wait().
- Add unit tests for CinTableBase in tests/test_cintablebase.py.

This eliminates CPU-spinning busy-wait polling loops and prevents worker threads from mutating shared frontend state.

Architecture and design by Hong Jen Yee (PCMan). Implementation generated with AI assistance from Google Antigravity (Gemini 3.8 Flash).
@PCMan
PCMan force-pushed the feature/cinbase-table-manager branch from 7596e44 to b74d6a2 Compare October 25, 2026 06:50
@PCMan PCMan changed the title feat(cinbase): add CinTableBase and replace thread with future feat(cinbase): asynchronous table loading with Future and CinTableBase Oct 25, 2026
@PCMan
PCMan requested a review from KenLuoTW October 25, 2026 06:50
@PCMan PCMan self-assigned this Oct 25, 2026
@PCMan

PCMan commented Oct 25, 2026

Copy link
Copy Markdown
Member Author

Hi @KenLuoTW I made some improvements for the CIN-base input methods. Could you help me review, please? Thanks!

@PCMan

PCMan commented Oct 25, 2026

Copy link
Copy Markdown
Member Author

重構的核心價值與改進重點

  1. 消除競態條件(Race Condition):

    • 舊版:Thread.start() 呼叫後,必須等待作業系統排程並真正執行到背景執行緒的 run(),loading = True 才會生效,中間存在短暫時間差,容易導致其他檢查誤判為「未在載入中」。
    • 新版:主執行緒提交工作時即取得 Future,loading 當下立即精準判定為 True。
  2. 徹底根除 CPU 盲等空轉(Busy-waiting):

    • 舊版:各輸入法模組使用 while CinTable.loading: continue 盲輪詢,佔用 100% CPU 單核並卡死 Python GIL。
    • 新版:改用 CinTable.wait()(底層以 Future 事件通知等待),完全不浪費 CPU 資源,效能與反應速度大幅提升。
  3. 執行緒安全與單向資料流:

    • 舊版:Worker 背景執行緒直接跨執行緒修改前端主物件(cbTS.cin),容易造成共享狀態髒讀與競爭。
    • 新版:背景執行緒只做純檔案 IO 與 JSON 解析,載入完成後透過回呼安全交付結果,實現解耦。
  4. 狀態可靠與例外透明:

    • 舊版:若背景執行緒崩潰或靜默吞掉錯誤,狀態變數容易卡死在非預期值。
    • 新版:Future 狀態由 Python 核心管理,無論成功或失敗 done() 均保證切換,且載入失敗時的 Exception 能正確在 wait() 中拋出,便於排查問題。

(Comment generated with assistance from Antigravity + Gemini 3.8 Flash)

@asdf80730

asdf80730 commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Reviewed head b74d6a2 with the bundled Python 3.15.1. The PR's own suite passes (tests/test_cintablebase.py, 5/5), and #355's original repro (a copy of python/cinbase/cin/array30.cin dropped into the runtime json dir as <table>.json) was executed against this tree.

What the rewrite gets right. The four claims in the description all hold on the success path: start() submits and returns, so the first load is genuinely async (verified: CinTable.loading is True immediately after start(), nothing blocks); the while CinTable.loading: continue busy-waits are gone in all 8 cin IMEs; loading flips at submit time instead of whenever the thread got scheduled; and the worker no longer writes cbTS.cin mid-load. This is the right architecture — my notes are about the failure path only.

Merge condition (the #355 half this PR does not yet close). A failed table must be visible, not fatal. I executed four failure modes and the silent death is still there, in new forms:

  1. The completion callback swallows the failure again. LoadCinTable._on_done ends in except Exception: pass (python/cinbase/__init__.py:3401-3402), so a corrupt table produces no log line anywhere. wait() does re-raise (cintablebase.py:50-61, pinned by test_wait_raises_task_exception), but every consumer that doesn't call wait() gets the exception from an unexpected place:

    • CinTable.cin property re-raises via future.result() (cintablebase.py:44-48), so the plain None-guards blow up when the future they guard is a failed one: measured directly, RCinTable.cin is None (:3292) and HCinTable.cin is None (:3299) — reached with those two features enabled and their table load pointed at the corrupt file — and the resync cbTS.cin = CinTable.cin (:3324-3325, from the main-table repro) each raise JSONDecodeError inside the key/timer path.
    • The second TextService instance's __init__ runs CinTable.wait() unguarded (checj_ime.py:65, same in all 8 modules) — with a failed first load the raise lands in the constructor and the IME cannot be instantiated at all. That is fatal, not visible.
    • Ask: the cin property returns None for a failed future (expose error/failed separately); keep wait() raising only where the caller can handle it, and wrap the IME-__init__ wait() so a failed load falls back to an error path instead of killing createInstance. Log the swallowed exception in _on_done — python/server.py's append_error_log (python_backend.log) already reaches reporters.
  2. A failed load is never retried. start() copies curCinType, userExtendTable, priorityExtendTable, ignorePrivateUseArea onto CinTable at submit time (:3389-3392), while master set them only after a successful load. Measured: after a failed load CinTable.curCinType == cfg.selCinType, so checkConfigChange's drift checks (:3268-3290) see nothing and never submit again — the table stays dead permanently, silently. Ask: move the four assignments into _on_done's success branch; failure must leave the drift detectable.

  3. A failed reload leaves a stripped corpse serving input. start() calls self.cbTS.cin.__del__() at submit (:3386-3387), and Cin.__del__ (cin.py:68-77) deletes chardefs/keynames/cincount. If the new load then fails, cbTS.cin still points at that object and loading is already False, so the loading guards — filterKeyDown :322 (silent return True) and the 正在載入輸入法碼表 toast in onKeyDown :436-441 — never fire. Measured: after a failed reload cbTS.cin.chardefs == {} — typing yields zero candidates with no message, no exception, no retry, while a perfectly good table sits on disk. Ask: drop/__del__ the old table only after the new one loaded (in _on_done's success branch).

  4. The worker thread reads per-instance state directly. Beyond _load_extendtable (:3409-3418: getConfigDir()/getDataDir()/findFile()), _load_table itself reads cbTS.imeDirName and cbTS.ignorePrivateUseArea (:3425) and cfg.userExtendTable/priorityExtendTable/ignorePrivateUseArea (:3429-3432) on the executor thread. Once per-IME config isolation (fix(cinbase): give each input method its own config instance #1014) lands, cbTS.cfg is per-instance mutable state, so the worker should not touch it — snapshot every input the load needs at submit, the way selCinFile already is. Same reasoning for future table-affecting settings: the CinTable mirror attributes are the drift-detection surface, so keep them written only on success (point 2) or a config change becomes a one-shot silent failure.

One nit, non-blocking: the second-instance else: CinTable.wait() (checj_ime.py:65) waits with no timeout; a bounded wait(timeout) + reload fall-through keeps slow loads off the protocol::HANG_TIMEOUT path (PIMELauncher/src/protocol.rs:11 restarts the backend when a pending request hits 15 s).

CI / collision. ci / build-and-sign and AppVeyor (branch + pr) all SUCCESS on b74d6a2; MERGEABLE/CLEAN. No other open PR touches LoadCinTable/LoadRCinTable/LoadHCinTable; #917 is superseded for this file (see its thread), #1014 only touches config plumbing.

The design is mergeable as-is once a failed table surfaces a traceback to python_backend.log and a message to the user instead of dying silently — that is what #355 actually asked for. Happy to re-test the failure paths on the next push. @PCMan

(Review comment generated with assistance from omp (Qwen3.8-Flash-Next))

asdf80730 added a commit to asdf80730/PIME that referenced this pull request Oct 30, 2026
…d failure modes

Ticket 05 drove upstream PR EasyIME#1004: the review is posted
(comment 6506773490) with EasyIME#355's silent-death half stated as the merge
condition. The section 4.3 snapshot predated the review; this names what
was executed against head b74d6a2 — the four measured failure modes (the
_on_done swallow, the cin/wait() raise into the None-guards and the IME
constructor, submit-time config mirroring killing retry, the stripped
corpse table after a failed reload) — so frontier tickets 13-20 can
check the loader state from the doc instead of re-measuring.

Architecture and design by asdf80730. Implementation generated with AI
assistance from omp (Qwen3.8-Flash-Next).
asdf80730 added a commit to asdf80730/PIME that referenced this pull request Oct 30, 2026
Reviewing EasyIME#1004 needed to run the PR's own tests/test_cintablebase.py, which
lives only in the worktree; the driver resolved test modules by basename from
the integration checkout only and died ModuleNotFoundError, forcing a manual
interpreter invocation that skipped the binding check. The driver now also
discovers from the test file's own directory, and a new self-test pins both
the discovery and the no-cinbase abort on both gates.

Architecture and design by asdf80730. Implementation generated with AI
assistance from omp (Qwen3.8-Flash-Next).
@PCMan
PCMan requested a review from ottokang November 1, 2026 07:53

This branch has not been deployed

No deployments
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