assets: harden tapd client lifecycle and RFQ handling - #1189
Conversation
Close the TapdClient when daemon initialization fails, during normal shutdown, and after the view command completes. This prevents gRPC transport resources from leaking across embedded daemon lifecycles and error paths.
Convert the configured duration once during client creation. Round positive fractional durations up to the whole seconds accepted by tapd. Reject zero, negative, and overflowing values, and cover the conversion boundaries with unit tests.
Restrict the asset-name cache mutex to map access so a slow QueryAssetStats call cannot block cached readers. Use an RWMutex for independent cache reads and add a concurrent regression test.
Validate the rate pointer and decimal coefficient before converting asset units. Return errors for nil, malformed, non-positive, and oversized-scale rates instead of allowing nil dereferences or division-by-zero panics. Add regression tests for each case.
25dd676 to
d8910ea
Compare
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
LGTM! 💾
Left come comments. The main one is hardening for GetRfqForAsset.
| if d.assetClient != nil { | ||
| d.assetClient.Close() | ||
| d.assetClient = nil | ||
| } |
There was a problem hiding this comment.
I propose to add defer inside if d.cfg.Tapd.Activate block above and close it there.
| // seconds accepted by tapd. Sub-second precision is rounded up so a positive | ||
| // timeout can never become tapd's invalid zero value. |
There was a problem hiding this comment.
Isn't there a stronger reason to do it so we don't get a timeout error?
If we account on 1.5s timeout (and actively using it, e.g. use it at time 1.4s, i.e. 0.1s before the timeout), but tapd enforces the timeout of just 1s (rounded down, as before), we get a timeout error. So we round up to request enough time, right?
| } | ||
| if coefficient.Sign() <= 0 { | ||
| return 0, fmt.Errorf("asset rate coefficient must be positive") | ||
| } |
There was a problem hiding this comment.
We need a similar hardening for GetRfqForAsset. Here is an example how to fix it: https://gist.github.com/starius/c23dd24330c3d36c9e852d2f4596313f
|
|
||
| c.assetNameMutex.Lock() | ||
| defer c.assetNameMutex.Unlock() | ||
| assetIdStr := hex.EncodeToString(assetId) | ||
| c.assetNameMutex.RLock() | ||
| if name, ok := c.assetNameCache[assetIdStr]; ok { | ||
| c.assetNameMutex.RUnlock() | ||
| return name, nil | ||
| } | ||
| c.assetNameMutex.RUnlock() | ||
|
|
There was a problem hiding this comment.
Let's preserve the defer pattern somehow. Maybe make private accessor methods for this map? To encapsulate map+mutex work there.
2 participants
Summary
Improve the tapd client’s lifecycle, timeout handling, concurrency, and validation.
viewcompletion.