Fix: Spring 7 Webflux NoSuchElementError on HttpHeaders#entrySet() - #4556
Fix: Spring 7 Webflux NoSuchElementError on HttpHeaders#entrySet() #4556mtomik wants to merge 11 commits into
Conversation
|
💚 CLA has been signed |
🤖 GitHub commentsJust comment with:
|
github-actions
Bot
added
agent-java
community
mtomik
changed the title
09b0d62 to
4fb8061
Compare
|
thanks for the review 👍 the failed build was due to incompatibility of that sub cancel with java 7. I reworked that using Instrumentation - |
4569cfe to
92bfe51
Compare
|
Tick the box to add this pull request to the merge queue (same as
|
| } | ||
|
|
||
| @Override | ||
| public ElementMatcher<? super TypeDescription> getTypeMatcher() { |
There was a problem hiding this comment.
Is there a common name to all the implementations we need to instrument here ? the hasSuperType is very expensive without getTypeMatcherPreFilter, so if possible adding getTypeMatcherPreFilter would be relevant here.
There was a problem hiding this comment.
only I can think of is something like this, but that probably will not narrow it much, right?
@Override
public ElementMatcher<? super NamedElement> getTypeMatcherPreFilter() {
return nameContains("Subscription").or(nameContains("Subscriber"));
}or maybe have multiple these instrumentations by package/library that implements the CoreSubscriber ? or have just one for the implementations from reactor.core and the rest will be cleaned potentially by the GC later?
There was a problem hiding this comment.
I think this should be fine as a first step, do you have a list of all the classes that are being currently instrumented here to verify all of them match ? If those are spring classes I would expect them to stay consistently named. If those are user-provided then it might be too narrow and we need another approach.
There was a problem hiding this comment.
the problem is, that there can come any implementation of CoreSubscriber. if we miss some thanks to this filter, the TracedSubscriber will not remove it immediately from its contextMap / subscriptionMap.
if I understand it correctly, the memory leak caused by this was always handled by GC. in this newer spring 7 came newer reactor or other library that allowed us to spot it more easily? that was at least my thought...
I had here another solution to solve it - the wrapper around that subscription (ecde26e) , but I had there compile issue with the older java versions. so maybe just try to solve that to avoid using this instrumentation?
| @@ -0,0 +1,6 @@ | |||
| # set to DEBUG for easier test application debugging | |||
| logging.level.root=DEBUG | |||
There was a problem hiding this comment.
we probably don't need to have DEBUG by default here, this will make the test execution very verbose.
There was a problem hiding this comment.
it seems like it is just copy from the existing "testapp" module. so should I remove it also from there?
92bfe51 to
1a4387b
Compare
|
run docs-build |
4 participants
What does this PR do?
Since the original PR is stuck for some time,
I created another one with the same commits + a bit better solution for the core of this issue.
Also when I was running these newer tests I found one more issue:
Spring7ServerFunctionalInstrumentationTest#dispatchErrortest was failing due to TracedSubscriber not removing the reference of the subscription oncancel()fromcontextMap. so the solution might be theCancellationAwareSubscriptionthat is just the wrapper calling thediscardIf()after cancel.Is it possible that the issue was there even with older versions of reactor (from older Spring version), but now in that Spring 7 ( reactor 3.8.6 ) it was always failing.
Checklist