Switch to using Span concurrent weak map in Vert.x 3 #2322
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
💚 Flaky test reportTests succeeded. 🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
AlexanderWert
left a comment
AlexanderWert
left a comment
There was a problem hiding this comment.
LGTM.
Minor question about unit test for context clean-up
| @Override | ||
| public ElementMatcher<? super MethodDescription> getMethodMatcher() { | ||
| return named("doEnd").and(takesNoArguments()); | ||
| return named("doEnd").or(named("onEnd")).and(takesNoArguments()); |
There was a problem hiding this comment.
did we miss this initially? onEnd is available in 3.9 but not in 3.6.
How did you notice this?
Maybe we need a unit test for making sure context gets cleaned up properly, WDYT?
There was a problem hiding this comment.
This is exactly what this map migration does- it triggers our existing global recycling test assertions, thus uncovering such cases.
What we had before is a map that holds references to spans without counting these references, so the Vert.x API could change, causing spans to remain in the map and still get recycled. Now that we count these map references, if you don't properly remove them from the map, the recycling assertion causes the v3-latest tests to fail, which is how I found that.
2 participants
Implementing #2092 for Vert.x 3
Switching to the dedicated span concurrent map helped to discover a small transaction leak (not causing memory leak because GC still cleans) due to a change in internal API.