Cleanup of dead mapBlockSource entries that aren't needed in FinalizeNode
ClosedPublic
Actions

Authored by CCulianu on Jul 24 2017, 16:40.

Details

Reviewers

deadalnix
freetrader

Group Reviewers

Restricted Project

Commits

rSTAGING75b99eb1cb9d: Cleanup of dead mapBlockSource entries that aren't needed in FinalizeNode
rABC75b99eb1cb9d: Cleanup of dead mapBlockSource entries that aren't needed in FinalizeNode

Summary

A tiny memory leak exists in net_processing.cpp, this patch fixes it

Test Plan

Compile and run bitcoin. No log messages are generated to verify this is working, but if you wish, you can add some to FinalizeNode() to see stale mapBlockSource entries accumulate as bitcoin runs.

Diff Detail

Repository

rABC Bitcoin ABC

Branch

master

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 576
Build 576: arc lint + arc unit

Event Timeline

CCulianu created this revision.Jul 24 2017, 16:40

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 24 2017, 16:40

@CCulianu : any way to accelerate the leak for testing purposes?

Good question. I'll investigate that and if it's the case that it can be accelerated somehow (maybe using test suite using a specifically constructed test?), I will either add a test case or figure out how it's reproduced.

Bump.

I can't figure out where they leak -- but I do know it's related to the crash we had before (for which you had to add a guard to check a stale mapBlockSource entry).

I do know the only place they can get cleaned up is in the BlockSignals().BlockChecked() call (which ends up being BlockChecked() in net_processing.cpp)... so some code path isn't triggering this signal under normal operation.

I'm going to bump this and request we merge it. It does no harm, costs us nothing, and if anything it saves a few bytes of leaked memory.

It's sad I can't actually characterize *why* it's happening right now.. :/

This require a test.

src/net_processing.cpp
320	braces
323	If you are going to increment it every time, you should do it in the for where it is usually done. Then you wouldn't need a else at all.

This revision now requires changes to proceed.Aug 4 2017, 16:57

OK, will come up with a test..

src/net_processing.cpp
320	FINE! :-\|
323	That's the usual pattern for erasing from a map while iterating. You can't increment an iterator after removing it from a map. This is the only way to really do it, AFAIK.

I don't see an obvious/easy way to test this. This is all about an internal data structure that sometimes leaks under very specific conditions.

I'm requesting you allow this change in.

Also: about the braces -- we need to talk about that. I advise you to allow flexibility in the style guide regarding braces. I think for obvious simple one-liners like this braces aren't necessary. You risk alienating potential contributors to the project with such draconian coding style rules, if you don't mind my saying so.

Anyway bump...

Ok for the test. Please update the loop to use remove_if if possible.

src/net_processing.cpp
323	I see. This is error prone. Maybe it is best to just use remove_if then.

Unfortunately no remove_if on std::map...

I'll add braces to please you, despite my better judgement. :)

CCulianu added inline comments.Aug 5 2017, 21:25

src/net_processing.cpp
323	Are you referring to my code as error-prone or the C++ way of doing it as being error prone? It's the boiler plate way in C++ to erase while iterating through a map. Most experienced C++ programmers have seen it a thousand times. So this construct is not any more error prone than any other code running somewhere in the universe that erases while iterating from a map. It's just std::map's own quirks here. Now you may be able to build an argument for this use case of std::map being error prone, or perhaps poorly thought out. Maybe the designers had their reasons for invalidating iterators after erasing (in fact I know they have -- it's faster and cheaper and C++ is all about being fast).