Page MenuHomePhabricator

[alias-server] Prevent reentrency in handleBlockConnected
ClosedPublic

Authored by bytesofman on Apr 12 2023, 17:00.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABC21a1e99e684d: [alias-server] Prevent reentrency in handleBlockConnected
Summary

T3060

Implement async-lock to prevent reentrancy in handleBlockConnected

If handleBlockConnected is called by parseWebsocketMessage and is already processing, the next call will wait until the first call completes

Test Plan

Review new unit test
npm test
Note: this unit test is something of a stub right now. It will matter more when we are actually updating and changing the database. This is where we will be able to actually verify no reentrancy.

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien requested changes to this revision.Apr 12 2023, 19:35
Fabien added a subscriber: Fabien.
Fabien added inline comments.
apps/alias-server/src/chronikWsHandler.js
66 ↗(On Diff #39639)

I don't feel like this log is very useful. I tend the keep the logs minimalist and mostly returning the errors

apps/alias-server/test/chronikWsHandlerTests.js
191 ↗(On Diff #39639)

If you want to test the lock you probably need to return false so it loops for some time

This revision now requires changes to proceed.Apr 12 2023, 19:35
bytesofman marked an inline comment as done.

remove debug logging

apps/alias-server/src/chronikWsHandler.js
66 ↗(On Diff #39639)

Yes, this is a debug log so I could check if the dependency was even working. More important to check this when we have actual async stuff happening in the loop where reentrancy would have an impact. Removed.

apps/alias-server/test/chronikWsHandlerTests.js
191 ↗(On Diff #39639)

For now, this still doesn't really matter. Both loops will still return the correct info.

Later on this test will be expanded to include database reads and writes, so that reentrancy would return a different value if it occured.

This revision is now accepted and ready to land.Apr 13 2023, 08:31