ledger: rearrange blockqueue start/stop#4964
Conversation
remove stale test interface
Codecov Report
@@ Coverage Diff @@
## master #4964 +/- ##
==========================================
- Coverage 53.64% 53.63% -0.01%
==========================================
Files 432 432
Lines 54068 54080 +12
==========================================
+ Hits 29003 29006 +3
- Misses 22814 22820 +6
- Partials 2251 2254 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
|
||
| if !bq.running { | ||
| close(bq.closed) | ||
| bq.closed = nil |
There was a problem hiding this comment.
Is setting this to nil necessary? Keep it closed, and replace it with a new one when restarted.
There was a problem hiding this comment.
receiving from nil blocks forever, and creating a chan and immediately closing it feels silly, so I like the nil checking
There was a problem hiding this comment.
Based on my suggestion, I expect that none of what you arguing should happen.
I must be missing something, can you please point to me when:
bq.closedwill be nil- where
bq.closedwill be created and immediately closed
| if bq.closed != nil { | ||
| // a previus close() is still waiting on a previous syncer() to finish | ||
| oldsyncer := bq.closed | ||
| bq.mu.Unlock() | ||
| <-oldsyncer | ||
| bq.mu.Lock() | ||
| } |
There was a problem hiding this comment.
If bq.closed is not set to nil, this can be simplified:
| if bq.closed != nil { | |
| // a previus close() is still waiting on a previous syncer() to finish | |
| oldsyncer := bq.closed | |
| bq.mu.Unlock() | |
| <-oldsyncer | |
| bq.mu.Lock() | |
| } | |
| // a previus close() is still waiting on a previous syncer() to finish | |
| bq.mu.Unlock() | |
| <-bq.closed | |
| bq.mu.Lock() |
algonautshant
left a comment
algonautshant
left a comment
There was a problem hiding this comment.
It looks correct.
Some suggestions to simplify and clarify the interfaces.
| oldsyncer := bq.closed | ||
| bq.mu.Unlock() | ||
| <-oldsyncer | ||
| bq.mu.Lock() |
| }() | ||
| defer bq.mu.Unlock() | ||
|
|
||
| if !bq.running { |
There was a problem hiding this comment.
| if !bq.running { | |
| if bq.running { | |
| // log a warning, we should not be restarting running block queue | |
| return | |
| } |
4 participants
Summary
Rearrange blockqueue start/stop to be cleaner and never nil inside ledger.go
Bonus: Remove stale test interface
Test Plan
Passes local unit tests.