Open codefromthecrypt opened 5 years ago
The check for the completed flag in AsyncListener can also be racy as multiple threads could concurrently enter the callbacks but the flag is only set to true after the method has been executed. Could be fixed with AtomicBoolean.compareAndSet(false, true)
.
Yeah this makes sense. Thanks for the heads-up.
Also @adriancole not sure if you saw this: https://github.com/Nike-Inc/wingtips/pull/82#discussion_r228643265 - a subtlety that causes the wrong HTTP response status code to be set on the span when onTimeout()
or onError()
are called.
Also (also!) yet another one more thing - if you're using undertow as your servlet container it completely blows away request attributes before onComplete()
is called: https://issues.jboss.org/browse/UNDERTOW-1437, screwing up lots of stuff you'd want to do when handling spans for async servlet calls.
@nicmunroe
a subtlety that causes the wrong HTTP response status code to be set on the span when onTimeout() or onError() are called.
I saw that behaviour in all servlet containers I tested, thanks for the pointer in your great comments. However, you would want to keep the Throwable you get at onError
-
https://github.com/Nike-Inc/wingtips/blob/31eaeb4598e5d86150905b34ae269d6dea318fc9/wingtips-servlet-api/src/main/java/com/nike/wingtips/servlet/WingtipsRequestSpanCompletionAsyncListener.java#L84, as it doesn't come with the event you get on the subsequent onComplete
in all those containers as well
@eyalkoren I've been able to call event.getThrowable()
in onComplete()
and get the associated throwable in the containers I've tested. ... Which might just be one container, I can't remember for sure how much I tested different containers. :) At the very least I did a sanity check just now using some tests from wingtips it works in Jetty 9.3.21. 🤷♂️
What container(s) does it not work with?
(It's frustrating that we get such wildly different behavior on different servlet containers.)
@nicmunroe
It's frustrating that we get such wildly different behavior on different servlet containers
Totally agree! It's especially so with the async APIs - see this comment in the corresponding PR as to what I tested and how different behaviour was.
I tested two error scenarios - one where AsyncConstext.dispatch
is used and the dispatched servlet ends with throwing ServletException
and the other where AsyncContext.start
is used with a Runnable
that ends throwing a RuntimeException
.
Oddly, Jetty was the worse of them- neither version even invoked onError
in either scenario. While versions 9.2 and 9.3 (not sure which minor) did not include a valid instance for event.getThrowable
when invoking onComplete
, I now noticed that a valid Throwable
arrived with the onComplete
in 9.4, so I assume it was changed in some late 9.3 version.
looks like a fun issue is becoming funner ? 🗡
Yes, I had the time of my life with it 😄
noting that this isn't done yet
I was pinged on https://github.com/elastic/apm-agent-java/pull/385 by @felixbarny maybe we have something to adjust. cc also @nicmunroe my other servlet friend ;)