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

reduce transport channel timeout period by half #1998

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

Conversation

TalDerei
Copy link
Contributor

@TalDerei TalDerei commented Jan 26, 2025

Description of Changes

references prax-wallet/prax#277 and possibly pairs with #2003

after losing connectiontransportFailure.abort is triggered at the transport layer, poisoning the transport and closing the connection channel. The extension's service worker terminates shortly after, and the combination of the 60 second timeout in our channel transport + service worker's refractory period after resetting prevents an immediate reconnection. But after the timeout period, reloading the page re-injects the content scripts (setting up the connection listeners) and initializes a new connection handle.

basically, we can reduce the timeout period in our custom transport-dom package or manually call chrome.runtime.reload() to force an extension restart (similar to locking the wallet which resets the service worker).

the tricky thing here is properly allowing the timeout to fully expire, which clears the transport channel and enables reconnection. however, interrupting it by (as an experiment) repeatedly auto-reloading the page (ie. trying to reconnect to the poisoned transport) will reset the timeout period...

Screen.Recording.2025-01-26.at.2.05.13.PM.mov

however, waiting out the timeout period (60 seconds) enables successful reconnection. this means connection errors will eventually resolve on their own, but we can reduce the time in half (30 seconds) required to re-establish the connection, making reconnections more responsive.

there's nothing in theory that prevents us from doing this.

Related Issue

references prax-wallet/prax#277

Checklist Before Requesting Review

  • I have ensured that any relevant minifront changes do not cause the existing extension to break.

@TalDerei TalDerei self-assigned this Jan 26, 2025
Copy link

changeset-bot bot commented Jan 26, 2025

🦋 Changeset detected

Latest commit: 74821d9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@penumbra-zone/transport-dom Minor
minifront Patch
@penumbra-zone/client Major
@penumbra-zone/services Major
@penumbra-zone/transport-chrome Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@TalDerei TalDerei marked this pull request as draft January 26, 2025 23:15
@TalDerei TalDerei marked this pull request as ready for review January 27, 2025 00:07
@TalDerei TalDerei requested a review from a team January 27, 2025 04:44
@TalDerei
Copy link
Contributor Author

TalDerei commented Jan 27, 2025

note to reviewers: more sufficient testing required! especially for slower internet connections (which we can simulate by throttling the network)

@VanishMax
Copy link
Contributor

VanishMax commented Jan 27, 2025

@TalDerei Can the network change JS event help deal with internet interruption? https://developer.mozilla.org/en-US/docs/Web/API/Navigator/onLine

pseudocode:

window.addEventListener("offline", (e) => {
  saveState();
});

window.addEventListener("online", (e) => {
  resumeWork();
});

@TalDerei TalDerei marked this pull request as draft January 28, 2025 20:52
@turbocrime
Copy link
Collaborator

turbocrime commented Feb 4, 2025

discussed during call:

network dependency is strange because transport init shouldn't rely on a network request. i'll investigate that

reducing timeout will likely have a negative effect on reliability. presently, the service worker is very single-threaded and some long running options block rpc response, particularly block processing and especially genesis sync. a longer timeout will tolerate longer blocking. this should improve as optimization efforts proceed.

transport timeout is an option passed during creation and could be easily updated. instead of updating the default which is consistent with connectrpc defaults, it should be updated in more penumbra-specific configs.

i think we typically use a transportOptions exported from one of the packages for consistency, it could be updated there, or in any specific createChannelTransport call where a longer timeout is desired.

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.

3 participants