Skip to content

Use more reliable control flow for child safeTranslator objects#35

Open
AbeJellinek wants to merge 1 commit into
zotero:masterfrom
AbeJellinek:child-translator-complete
Open

Use more reliable control flow for child safeTranslator objects#35
AbeJellinek wants to merge 1 commit into
zotero:masterfrom
AbeJellinek:child-translator-complete

Conversation

@AbeJellinek

Copy link
Copy Markdown
Member
  • Make sure that Item#complete() is set to the parent translator's version of that method before returning from translate().

    This was already the case in practice, but for a weird reason: there's no way to get a usable (i.e. un-completed) item object from a child translator without overriding the default itemDone handler, and safeTranslator wraps your handler in some code that overwrites complete(). So even adding an empty handler - translation.setHandler('itemDone', (_obj, _item) => {}) - invisibly causes the item objects passed to it to be modified so that a later (await translation.translate())[0].complete() works. That's really odd and hard to debug. Now we're explicit about modifying the item object in all cases where it's returned to the parent.

  • translate.decrementAsyncProcesses("safeTranslator#translate()") was being called from a done handler that was only added once, the first time translate() got called. That caused translation never to complete if the parent translator called a child translator's translate() multiple times (not sure if anyone actually does that), but more critically, also in cases where done handlers never got triggered at all. Apparently done handlers never get triggered when the translator ID passed to setTranslator() doesn't resolve (deleted translator, typo, ...), so translation was hanging in that case. Now we decrement in finally(), which is much more reliable.

@AbeJellinek AbeJellinek requested a review from adomasven March 21, 2024 19:42
}
return returnValue;
})
.finally(() => translate.decrementAsyncProcesses("safeTranslator#translate()"));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's use try {} finally {} with an let returnValue = await translation.translate() instead of then.

@adomasven

Copy link
Copy Markdown
Member

Can we have a testcase for this? It would be easier to follow the logic than the explanation above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants