brave
brave copied to clipboard
openzipkin
investigate if we need to improve asynchronous handling of servlet
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 ;)
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.