[undertow-dev] FormDataProcessor ending exchange prematurely
Stuart Douglas
sdouglas at redhat.com
Thu Jan 26 21:09:37 EST 2017
On Fri, Jan 27, 2017 at 12:10 PM, Oliver Dain <oliver at analyticspot.com> wrote:
> Along these lines, I've been trying to figure out a reliable way to tell if
> a call to dispatch is necessary. Suppose I have a handler that can be called
> alone or could be nested inside another handler. My handler is going to do
> something async so I need to make sure it's been dispatched. Since it's
> about to do something very fast and async there's no need to move that work
> to another thread, I just want to run it locally so **if** it needs
> dispatching I'd call "dispatch(SameThreadExecutor.INSTANCE, ::doSomeStuff)".
> The handler wrapping this might do the same (dispatch, but right back to the
> IO thread). Thus, when my handler gets called it **might** have already been
> dispatched but it's not sure.
If the exchange has already been dispatched (i.e. it is no running
inside Connectors.executeRootHandler) then calling
dispatch(SameThreadExecutor.INSTANCE, ::doSomeStuff) is very fast, it
will basically just execute the task in the provided executor (in
general any SameThreadExecutor dispatch is very fast, the cost of a
dispatch is from dispatching to a real executor).
>
> Calling "Exchange.isInIoThread()" isn't reliable as the wrapping handler may
> have dispatched via SameThreadExecutor so being on the IO thread does not
> mean a dispatch in needed. OTOH, as I understand it (not sure, please
> correct me if I'm wrong) "isDispatched" isn't generally reliable as that
> gets unset as soon as you've left the IO thread so if the wrapper dispatched
> using any executor besides SameThreadExecutor my handler doesn't need to
> dispatch but isDispatched will be false.
>
> So, I think what is reliable is that you need to dispatch if and only if
> "isInIoThread() && !isDispatched()". Is that correct?
If you are going to use SameThreadExecutor you might as well just
always dispatch, it should not make any difference performance wise.
Stuart
>
> On Thu, Jan 26, 2017 at 4:10 PM Stuart Douglas <sdouglas at redhat.com> wrote:
>>
>> In general there is never any real need to dispatch back to an IO
>> thread, although if you perform any non blocking IO ownership of the
>> exchange can end up back in the IO thread when the operation is
>> complete.
>>
>> Stuart
>>
>> On Thu, Jan 26, 2017 at 12:53 AM, Bill O'Neil <bill at dartalley.com> wrote:
>> > Hey Stuart, your last response reminded me of of a question I had
>> > hopefully
>> > its not unrelated. I was curious how and when you should dispatch back
>> > to
>> > the IO thread. For example lets say we have a simple handler that does a
>> > SQL
>> > query that is composed with the AccessLogHandler.
>> >
>> > AccessLogHandler -> BlockingHandler -> CustomSqlHandler In this approach
>> > I
>> > believe the final access log statement will be logged from the worker
>> > thread
>> > not the initial IO thread.
>> >
>> > Is it possible / recommended to kick back to an IO thread once blocking
>> > is
>> > complete?
>> >
>> > AccessLogHandler -> BlockingHandler -> CustomSqlHandler ->
>> > DispatchBackToIOThread? This way the final logging of AccessLogHandler
>> > is
>> > handled in the IO thread.
>> >
>> > I'm not very familiar with the best ways to mix the nonblocking and
>> > blocking
>> > handlers.
>> >
>> >
>> >
>> > On Wed, Jan 25, 2017 at 12:24 AM, Stuart Douglas <sdouglas at redhat.com>
>> > wrote:
>> >>
>> >> The handler is executed
>> >> io.undertow.server.Connectors#executeRootHandler, which means that
>> >> when the call stack returns the exchange will be ended.
>> >>
>> >> Conceptually this is similar to how dispatching to a thread pool
>> >> works, when you call dispatch(HttpHandler) the exchange will be ended
>> >> when the call stack returns, even though you are no longer on the IO
>> >> thread (unless you call dispatch again).
>> >>
>> >> Stuart
>> >>
>> >>
>> >> On Wed, Jan 25, 2017 at 3:57 PM, Oliver Dain <oliver at analyticspot.com>
>> >> wrote:
>> >> > I have some code for handling file uploads. It uses FormDataProcessor
>> >> > and
>> >> > tries to do everything asynchronously. I call "FormDataParser.parse"
>> >> > passing
>> >> > in another handler. I'll call that handler the OnFormDataAvailable.
>> >> > The
>> >> > OnFormDataAvailable handler checks to see if it's on the IO thread.
>> >> > If
>> >> > it
>> >> > is, it calls dispatch. Either way, once we're sure we're dispatched
>> >> > that
>> >> > handler calls yet another handler.
>> >> >
>> >> > What I've seen is that my OnFormDataAvailable handler (the one called
>> >> > by
>> >> > parse()) is not on the IO thread so it doesn't need to call dispatch.
>> >> > And
>> >> > yet, the exchange gets ended before the handler it calls is even
>> >> > close
>> >> > to
>> >> > complete.
>> >> >
>> >> > I've found a fix, but I don't understand why it's necessary.
>> >> > Specifically,
>> >> > if OnFormDataAvailable is not on the IO thread when its invoked it
>> >> > calls
>> >> > the
>> >> > 0-argument version of "HttpServerExchange.dispatch()" (which you've
>> >> > told
>> >> > me
>> >> > in another conversation shouldn't ever be necessary). If I do that,
>> >> > everything is fine.
>> >> >
>> >> > For completeness, here's the complete code:
>> >> >
>> >> > public class FormDataParsingHandler {
>> >> > private static final Logger log =
>> >> > LoggerFactory.getLogger(FormDataParsingHandler.class);
>> >> > private static final FormParserFactory formParserFactory =
>> >> > FormParserFactory.builder().build();
>> >> > public static final AttachmentKey<FormData>
>> >> > FORM_DATA_ATTACHMENT_KEY =
>> >> > AttachmentKey.create(FormData.class);
>> >> >
>> >> > /**
>> >> > * The only public method - this is what gets exposed as the
>> >> > HttpHandler.
>> >> > */
>> >> > public CompletableFuture<FormData> parseForm(HttpServerExchange
>> >> > exchange)
>> >> > {
>> >> > log.info("audio file upload request received.");
>> >> > FormDataParser parser = formParserFactory.createParser(exchange);
>> >> > if (parser == null) {
>> >> > log.warn("No parser found that can handle this content type.
>> >> > Headers
>> >> > were: {}", exchange.getRequestHeaders());
>> >> > throw new UserVisibleException("No parser for the given content
>> >> > type.", ResponseCodes.BAD_REQUEST);
>> >> > }
>> >> >
>> >> > CompletableFuture<FormData> toComplete = new
>> >> > CompletableFuture<>();
>> >> > try {
>> >> > parser.parse(new OnFormDataAvailable(toComplete));
>> >> > } catch (Exception e) {
>> >> > log.error("Error parsing form data:", e);
>> >> > throw wrapAsUnchecked(e);
>> >> > }
>> >> >
>> >> > exchange.addExchangeCompleteListener((ex, nextListener) -> {
>> >> > // Must close the parser so it can free any temporary files
>> >> > that
>> >> > were
>> >> > created.
>> >> > try {
>> >> > parser.close();
>> >> > } catch (IOException e) {
>> >> > log.error("Error closing the FormDataParser. Request was
>> >> > handled
>> >> > successfully but temporary files may not "
>> >> > + "have been cleaned up.", e);
>> >> > }
>> >> > nextListener.proceed();
>> >> > });
>> >> >
>> >> > return toComplete;
>> >> > }
>> >> >
>> >> > // The FormDataParser calls an HttpHandler when it's complete so we
>> >> > add a
>> >> > silly handler here that does nothing but
>> >> > // complete this method's future when the form data is available.
>> >> > private static class OnFormDataAvailable implements HttpHandler {
>> >> > private final CompletableFuture<FormData> toComplete;
>> >> >
>> >> > private OnFormDataAvailable(CompletableFuture<FormData>
>> >> > toComplete)
>> >> > {
>> >> > this.toComplete = toComplete;
>> >> > }
>> >> >
>> >> > @Override
>> >> > public void handleRequest(HttpServerExchange exchange) throws
>> >> > Exception
>> >> > {
>> >> > // Before we complete the future we have to re-dispatch or
>> >> > we'll
>> >> > fall
>> >> > off the end of this method and Undertow
>> >> > // will complete the exchange on our behalf.
>> >> > FormData data =
>> >> > exchange.getAttachment(FormDataParser.FORM_DATA);
>> >> > if (exchange.isInIoThread()) {
>> >> > log.debug("Was on the IO thread. Re-dispatching.");
>> >> > exchange.dispatch(SameThreadExecutor.INSTANCE, () ->
>> >> > afterDistpach(data));
>> >> > } else {
>> >> > // THIS THE MYSTERY LINE. WHY IS THIS NEEDED?
>> >> > exchange.dispatch();
>> >> > afterDistpach(data);
>> >> > }
>> >> > }
>> >> >
>> >> > private void afterDistpach(FormData data) {
>> >> > if (data == null) {
>> >> > toComplete.completeExceptionally(
>> >> > new UserVisibleException("Parsing of data failed.",
>> >> > ResponseCodes.BAD_REQUEST));
>> >> > } else {
>> >> > toComplete.complete(data);
>> >> > }
>> >> > }
>> >> > }
>> >> > }
>> >> >
>> >> > --
>> >> > CTO, Analytic Spot
>> >> > 44 West Broadway #222
>> >> > Eugene, OR 97401
>> >> > analyticspot.com • 425-296-6556
>> >> > www.linkedin.com/in/oliverdain
>> >> >
>> >> > _______________________________________________
>> >> > undertow-dev mailing list
>> >> > undertow-dev at lists.jboss.org
>> >> > https://lists.jboss.org/mailman/listinfo/undertow-dev
>> >>
>> >> _______________________________________________
>> >> undertow-dev mailing list
>> >> undertow-dev at lists.jboss.org
>> >> https://lists.jboss.org/mailman/listinfo/undertow-dev
>> >
>> >
>
> --
> CTO, Analytic Spot
> 44 West Broadway #222
> Eugene, OR 97401
> analyticspot.com • 425-296-6556
> www.linkedin.com/in/oliverdain
More information about the undertow-dev
mailing list