feat: use one transaction per move item and send ws after the transaction#835
feat: use one transaction per move item and send ws after the transaction#835
Conversation
- move transaction into move function of itemService - publish websocket post move item after the transaction - publish websocket feedback after all the transactions
- each transaction execute one move - add ws services to publish websockets
ReidyT
changed the title
- check that the move is committed when ws is received
pyphilia
left a comment
pyphilia
left a comment
There was a problem hiding this comment.
Thanks for the PR! Here are some ideas to refactor the code, plus some questions 🐈
src/services/item/controller.ts
Outdated
| } | ||
| }).catch((e) => { | ||
| log.error(e); | ||
| Promise.allSettled<Item>( |
There was a problem hiding this comment.
Something that would be worth checking is whether all promises start at the same time, or if they are running one after the other. I think we prefer not sending them all at the same time. What do you think?
There was a problem hiding this comment.
According to the javascript documentation, The Promise class offers four static methods to facilitate async task [concurrency](https://en.wikipedia.org/wiki/Concurrent_computing), so it should be "in parallel".
They also indicates: Note that JavaScript is [single-threaded](https://developer.mozilla.org/en-US/docs/Glossary/Thread) by nature, so at a given instant, only one task will be executing, although control can shift between different promises, making execution of the promises appear concurrent.
So, maybe it is better to run in series as you mentioned here to be sure that there is no deadlock ?
src/services/item/service.ts
Outdated
| await this.hooks.runPreHooks('move', actor, repositories, { | ||
| source: item, | ||
| destinationParent: parentItem, | ||
| }); |
There was a problem hiding this comment.
So there's no prehook anymore?
There was a problem hiding this comment.
I didn't found one listening for move preHooks...
src/services/item/ws/services.ts
Outdated
| errors: [], | ||
| }, | ||
| ), | ||
| ); | ||
| } | ||
| if (failed.length) { | ||
| failed.forEach((e) => { | ||
| log.error(e); | ||
| websockets.publish( | ||
| memberItemsTopic, | ||
| memberId, | ||
| ItemOpFeedbackEvent('move', failedIds, { error: e }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Why not having one feedback with the errors within the "success" message and let the frontend trigger the multiple notifications? I know we discussed it sorry..
There was a problem hiding this comment.
Because there are some changes to do... but I can actually do this now.
- implement ItemWebsocketsService class - implement SeriesPromise class to run Promises in series - use newly postMoveAction function
|
2 participants
For more details, please see #831
Validation of the Implementation
Protocol of the Test:
Tests 1 - Parallel vs Series with Multiple Transactions without Intermediate Web Sockets
Results: Parallel seems to be slower than in series. Also, in series, the feedback isn’t sent at the end of the 20 items but at the whole end.
Important
In parallel, each promise has its own thread freezing the database for the time that the move is run. This is not acceptable.
Note
Maybe use a thread pool (running 5 threads, in each thread having 20 items to move in series…)
Test 2 - Only One Transaction, moving items are executed in parallel, without web sockets
Test 3 - Parallel vs Series with Multiple Transactions and web sockets
Important
Refresh the browser to be sure to have a web socket connection!!!
The problem principally comes from the rerender of the table… Indeed, when disabling the invalidation, the manual manipulations conduct to a re-render, involving the HTTP Requests.
Results: In parallel is a little bit faster.