wip: set up local journal #646
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for _, txs := range all { | ||
| for _, tx := range txs { | ||
| if err = rlp.Encode(replacement, tx); err != nil { | ||
| replacement.Close() | ||
| return err | ||
| } | ||
| } | ||
| journaled += len(txs) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
| for _, txs := range all { | ||
| for _, tx := range txs { | ||
| if err = rlp.Encode(replacement, tx); err != nil { | ||
| replacement.Close() |
Check warning
Code scanning / CodeQL
Writable file handle closed without error handling Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 13 days ago
To fix this issue, always check and handle errors returned by replacement.Close(). In this code, there are two places where replacement.Close() is called:
- In the error path inside the loop, after a failed
rlp.Encode - After the loop, when all transactions have been written
In both cases, the error from Close() should be checked. In the error path, if rlp.Encode fails, and replacement.Close() also fails, both errors are relevant, but returning the first error (the encoding error) is conventionally favored, though logging the close error could also be considered. In the success path, if replacement.Close() fails, its error should be returned to the caller instead of pretending everything succeeded.
The code should be updated to:
- Check the error from
replacement.Close(). - In the error path, attempt to close and return the error from
rlp.Encode(possibly logging any close error). - In the normal path, after
replacement.Close(), return any error from closing (if present).
No extra imports or new methods are needed, just additional error handling.
| @@ -147,13 +147,18 @@ | ||
| for _, txs := range all { | ||
| for _, tx := range txs { | ||
| if err = rlp.Encode(replacement, tx); err != nil { | ||
| replacement.Close() | ||
| closeErr := replacement.Close() | ||
| if closeErr != nil { | ||
| // Optionally, log closeErr here using `log.Warn`, since only one error can be returned. | ||
| } | ||
| return err | ||
| } | ||
| } | ||
| journaled += len(txs) | ||
| } | ||
| replacement.Close() | ||
| if err := replacement.Close(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Replace the live journal with the newly generated one | ||
| if err = os.Rename(journal.path+".new", journal.path); err != nil { |
| } | ||
| journaled += len(txs) | ||
| } | ||
| replacement.Close() |
Check warning
Code scanning / CodeQL
Writable file handle closed without error handling Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 13 days ago
General approach:
Explicitly check the error returned by replacement.Close() on line 156, and propagate it up by returning if it is non-nil.
Detailed steps:
- After the loop writing to
replacement, instead of callingreplacement.Close()and discarding its error, check it. - If this error is non-nil, return it immediately, just as is done for other errors within
rotate. - This preserves the existing functional behavior but closes the bug where a late error would be suppressed.
- No additional imports or helpers are needed. Just update the close call and error handling.
Where to change:
- In
mempool/txpool/locals/journal.go, in therotatefunction, around line 156 (after writing transactions and before renaming the file).
| @@ -153,7 +153,9 @@ | ||
| } | ||
| journaled += len(txs) | ||
| } | ||
| replacement.Close() | ||
| if err := replacement.Close(); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Replace the live journal with the newly generated one | ||
| if err = os.Rename(journal.path+".new", journal.path); err != nil { |
| for sender, txs := range tracker.byAddr { | ||
| // Wipe the stales | ||
| stales := txs.Forward(tracker.pool.Nonce(sender)) | ||
| for _, tx := range stales { | ||
| delete(tracker.all, tx.Hash()) | ||
| } | ||
| numStales += len(stales) | ||
|
|
||
| // Check the non-stale | ||
| for _, tx := range txs.Flatten() { | ||
| if tracker.pool.Has(tx.Hash()) { | ||
| numOk++ | ||
| continue | ||
| } | ||
| resubmits = append(resubmits, tx) | ||
| } | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
| for _, tx := range tracker.all { | ||
| addr, _ := types.Sender(tracker.signer, tx) | ||
| rejournal[addr] = append(rejournal[addr], tx) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
| for _, list := range rejournal { | ||
| // cmp(a, b) should return a negative number when a < b, | ||
| slices.SortFunc(list, func(a, b *types.Transaction) int { | ||
| return int(a.Nonce() - b.Nonce()) | ||
| }) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
| // layer was also initialized to spawn any goroutines required by the service. | ||
| func (tracker *TxTracker) Start() error { | ||
| tracker.wg.Add(1) | ||
| go tracker.loop() |
Check notice
Code scanning / CodeQL
Spawning a Go routine Note
| defer tracker.journal.close() | ||
| } | ||
| var ( | ||
| lastJournal = time.Now() |
Check warning
Code scanning / CodeQL
Calling the system time Warning
| if checkJournal { | ||
| // Lock to prevent journal.rotate <-> journal.insert (via TrackAll) conflicts | ||
| tracker.mu.Lock() | ||
| lastJournal = time.Now() |
Check warning
Code scanning / CodeQL
Calling the system time Warning
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
mainbranch