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

Refact native application connection closing #74

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

taneltm
Copy link
Member

@taneltm taneltm commented Jul 18, 2024

The way the NativeAppService worked before

  • Handshake
    • Extension opened the connection to the native app
    • Extension waited for a message from the native app which has the version using await nextMessage(...)
  • Native app waiting for commands
    • Extension could now send a command and wait for a reply and theoretically do that multiple times
    • Extension keeps waiting for the native app to disconnect

It was implemented this way, because initially there was an idea that we could get the signing certificate and request signing in the same native app instance. For various reasons, we decided it was better to start a new native app instance in such a case.
In the current code base, the background/actions don't support such a case anymore.

There was also a bug with the Edge browser which got a temporary fix in #67, however this should no longer be necessary.

The way the NativeAppService works now

  • Handshake
    • Extension opened the connection to the native app
    • Extension waited for a message from the native app which has the version using await nextMessage(...)
  • Native app waiting for commands
    • Extension sends a single command to the native app
    • Extension waits for a response from the native app, reusing await nextMessage(...)
    • Extension closes the native app port immediately, without waiting for the native app to close on it's own

The nextMessage now has a throwAfterTimeout: Error argument and the background\actions no longer handle the timeout logic on their own. This way, when the timeout occurs, the nextMessage(...) can run it's cleanup logic.

@mrts mrts self-requested a review August 2, 2024 17:14
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