Closed ritave closed 2 years ago
Currently the following timeouts exist in the relevant modules. They are all configurable, but we currently just use the defaults:
SnapController
:
maxRequestTime
, 60 seconds)maxIdleTime
, 30 seconds)AbstractExecutionService
(the base class for all execution services)
unresponsiveTimeout
, 30 seconds)ping
messages before it is terminated.There are also intervals in the above modules but they are implementation details that should not affect the relevant behaviors.
@ritave, I believe your recommendation is referring to the maxRequestTime
. Are the other timeouts related to this issue or should we consider them to be out of scope?
The Snap can execute setTimeout()
inside an RPC request, which will execute after the eval finishes. The setTimeout()
will be execute during idle time and so would have a 30 second timeout instead of the one reserved for RPC requests.
We talked with @rekmarks and decided that all timeouts systems needs to be refactored along with redesigning the snap's lifecycle
FYI, we encountered the same issue and recommend to make timeouts configurable: https://github.com/MetaMask/metamask-extension/pull/14256
We added endowment:long-running
permission. Closing
Currently, the timeouts are set to a low value of 60 seconds. It can interfere with long-lasting calculations, such as calculating an RSA-4096 key used in some blockchains. Simply updating timeouts to a higher value might also interfere with other calculations we might not foresee (basic example would be generation of vanity addresses).
Another basic use-case is using
snap_confirm
inside a rpc request. The RPC can timeout waiting for user response.Since all RPC in Snaps are only localhost, there can't be actual network problems interfering with the commands. Only reasons the RPC takes a long time is because there are either actual calculations taking place in a Snap or it has entered a bad state, such as an infinite loop, and is causing a service denial. Due to our previous testing of running 100 Snaps at the same time, neither of both reasons are an actual problem to the functioning of Metamask extension.
Thus my suggestion is:
This would be consistent with how browsers treat
fetch
calls. For example, Chrome has a default timeout of 300 seconds and cancels requests when the user navigates out of the page.