[PECO-618] Better handling of in-progress/subsequent action taken on closed session/operation #129
Conversation
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
…esponse only when operation is being closed Signed-off-by: Levko Kravets <levko.ne@gmail.com>
…losed, it still attempted to fetch data Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
kravets-levko
requested review from
arikfr,
superdupershant and
yunbodeng-db
as code owners
kravets-levko
requested review from
susodapop,
nithinkdb and
andrefurlan-db
as code owners
| public async closeAll() { | ||
| const items = [...this.items]; | ||
| for (const item of items) { | ||
| await item.close(); // eslint-disable-line no-await-in-loop |
There was a problem hiding this comment.
Could using an async closure and Promise.all() avoid disabling the eslint warning?
There was a problem hiding this comment.
I wasn't sure if that's safe to run all the close operations in parallel - if there will be a lot of objects in collection, server may reject some of them. So for me it looked safer to run them in sequence. Also, there's nothing wrong with disabling no-await-in-loop - docs even suggest doing that if developer is sure that promises should be executed in sequence
|
@andrefurlan-db it's not that complicated as it looks. Brief idea of what's implemented here: we use DBSQLClient to create instances of DBSQLSession, which later runs some operations returning DBSQLOperation instances. Previously that "nested" object were created standalone. So if you, for example, close connection (which in our case doesn't really close anything because it's a nature of HTTP), or close session (which doesn't actully close session immediately because that's how our backend works) - it was still possible to use those nested objects. However, users expected other behavior - if you close session, all operations that belong to it should be closed as well; same for connection - when you close it, you should invalidate everything that belongs to it. So what we do here: clients and sessions keep track of objects they create, and once being closed - they first close all owned object, and only then close themselves. It's very similar to what PySQL does, just a little bit more complicated: first because PySQL creates the only session under the hood so it's just a single level of nested objects, and secondly - because in Node a lot of things may happen asynchronously, and we need to handle that as accurately as possible. |
Signed-off-by: Levko Kravets <levko.ne@gmail.com>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #129 +/- ##
==========================================
+ Coverage 94.98% 95.11% +0.13%
==========================================
Files 57 58 +1
Lines 1076 1167 +91
Branches 178 191 +13
==========================================
+ Hits 1022 1110 +88
- Misses 21 22 +1
- Partials 33 35 +2
☔ View full report in Codecov by Sentry. |
| if (!this._status.hasResultSet) { | ||
| return []; | ||
| } | ||
|
|
||
| await this._status.waitUntilReady(options); | ||
| await this.waitUntilReady(options); |
There was a problem hiding this comment.
cool, I've wanted this change for a while
PECO-618
Asynchronous nature of NodeJs makes possible situations when user started working with some operation (e.g. fetching data), and at the same time the operation or even the corresponding session gets closed (sometimes it’s a result of race condition). Session also may expire after some time of inactivity, which also causes similar issue.
We need to properly handle cases when some action is performed on closed operation/session:
Checklist: