-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
server/msgBook: atomic seq updates with respect to order book contents #2
base: master
Are you sure you want to change the base?
Conversation
3083cf1
to
a6fb6f7
Compare
server/market/bookrouter.go
Outdated
book.mtx.RLock() | ||
defer book.mtx.RUnlock() | ||
|
||
if !book.running { | ||
// Don't want to block client requests for too long, so return fast if order book | ||
// isn't initialized yet instead of proceeding to wait on mutex below for who | ||
// knows how long. | ||
if !book.running.Load() { | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this was the original intent (see the comment I added there) for having running
flag there, it didn't work because client request would block on book.mtx
before order book is available or not. Or is there any other purpose for running
?
Or maybe it just got out of date. Moved running
from under book.mtx.RLock()/book.mtx.RUnlock()
section for now.
Just to clarify this is the expected behavior (it seems to work like that on master now):
|
ad9d264
to
e298e04
Compare
f9959eb
to
a66bbae
Compare
e3c487c
to
333e9f6
Compare
seq
updates on server formsgBook
are not atomic with respect to order book contents, this means 2 clients (or even 1 client sending 2 requests in parallel) can see different order book contents for the sameseq
number (e.g. one order book snapshot already including last book_order/update_remaining/unbook_order update effects, while the other snapshot lacking those because that update technically belongs to the nextseq
and is only there for a brief period of time because there is no atomicity there)which, in turn, means:
seq+1
afterwards); currently dexc resolves such redundant book_order/unbook_order notifications by checking order existence inOrderBook.orders
map but doesn't seem to do anything that would prevent update_remaining from applying twice (which is another way we can get to app: ignore zero-quantity book_order updates decred/dcrdex#1543)seq
andseq+1
); if server is running in VM and it gets frozen at just the right instruction for a sec ... to resume later ... or something like that; gotta be pretty unlikely to happen in practice, probably also off the table tooIt's not an issue per se?I think it is, see 1) above.Overall, it's a tradeoff that leaves more room for shooting yourself in the foot for seemingly no good reason.
This PR adds aforementioned atomicity property.