refactor(askar)!: short-lived askar sessions #1743
Conversation
Signed-off-by: Timo Glastra <[email protected]>
| try { | ||
| if (recipientKeyEntry) { | ||
| return didcommV1Unpack(messagePackage, recipientKeyEntry.key) | ||
| // TODO: how long should sessions last? Just for the duration of the unpack? Or should each item in the recipientKids get a separate session? |
There was a problem hiding this comment.
I don't know what's exactly the overhead of getting references to sessions in the pool (probably not much/nearly zero) but in this case I think it will become clearer if a single session is used for the whole unpack process (especially once we switch to the using keyword).
| forUpdate: false, | ||
| isJson: false, | ||
| // TODO: what is faster? listProfiles or open and close session? | ||
| // I think open/close is more scalable (what if profiles is 10.000.000?) |
There was a problem hiding this comment.
That would be an interesting stress test! Currently, list_profiles makes a single query (SELECT name FROM profiles) and returns all strings at once to FFI layer. In case of millions of records I guess there can be problems in different parts of the stack being used (e.g. maximum variable/array size, SQL query/response length, etc.).
So I think unless there is a new method in askar allowing to check if a particular profile exists, it is safer to do the open/close
| this.walletConfig = walletConfig | ||
| this._session = await this._store.openSession() | ||
| } catch (error) { | ||
| // FIXME: Askar should throw a Duplicate error code, but is currently returning Encryption |
There was a problem hiding this comment.
Just curious, do you know if this is still happening? There were quite a good number of fixes regarding this in Askar in the past months.
There was a problem hiding this comment.
No not sure, but we can find out :)
Signed-off-by: Timo Glastra <[email protected]>
TimoGlastra
force-pushed
the
feat/short-lived-askar-sessions
branch
from
175ca30
to
ed14a57
Compare
This should address some issues we've encountered with Askar where if a long-lived session dies the wallet is not usable anymore.
With askar, sessions are meant to be short-lived and then askar can manage a pool of connections.
This will also improve cases where a lot of tenant sessions are opened at the same time which could result in deadlocks as we had to keep a session open for each open tenant, while now we close the tenant session, even if the tenant agentContext is still open. In a future PR I'll probalby do some rework on the session mutex stuff as that becomes a lot less useful as opening a new tenant is as simple as opening a profile, so there's not really overhead in having a lot of sessions open unless DatabasePerWallet is used in multi-tenancy
In the future we can also start batching multiple reads/writes so that if it fails we can rollback and we won't have partially updated state (think adding CredentilaExchangeRecord + DidCommMessageRecord). but that is probably something for 0.6 as we need to revamp the Wallet / Session api for that. This is now just internally to askar.
Some breaking changes related to export/import as described here: hyperledger/aries-askar#221