Uploaded image for project: 'Undertow'
  1. Undertow
  2. UNDERTOW-2512

Empty response body regression with request body and delayed response

    • Icon: Bug Bug
    • Resolution: Not a Bug
    • Icon: Minor Minor
    • None
    • 2.3.18.Final
    • None
    • None
    • Hide

      To reproduce the issue:

      • Import the attached Spring Boot project
      • Run the application in the IDE or with `./gradlew bootRun`
      • Send the requests defined in the `test.http` file, POST and PUT requests will likely return an empty response, GET will provide the expected response

      To see the regular behavior:

      • Comment/remove ext["undertow.version"] = "2.3.18.Final" in build.gradle.kts in order to use Undertow 2.3.17.Final 
      • Refresh Gradle configuration in the IDE
      • Run the application in the IDE or with `./gradlew bootRun`
      • Send the requests defined in the `test.http` file, all POST, PUT and GET requests will return the expected response
      Show
      To reproduce the issue: Import the attached Spring Boot project Run the application in the IDE or with `./gradlew bootRun` Send the requests defined in the `test.http` file, POST and PUT requests will likely return an empty response, GET will provide the expected response To see the regular behavior: Comment/remove ext ["undertow.version"] = "2.3.18.Final" in build.gradle.kts in order to use Undertow 2.3.17.Final  Refresh Gradle configuration in the IDE Run the application in the IDE or with `./gradlew bootRun` Send the requests defined in the `test.http` file, all POST, PUT and GET requests will return the expected response
    • Migration

      Hi,
       
      While preparing the next release of Spring Framework and Spring Boot, I think we discovered a regression introduced by Undertow 2.3.18.Final. It looks like Reactive endpoints where the request body is read and the response is delayed ends up with an empty response body most of the time while it is was working as expected with Undertow 2.3.17.Final and previous versions.
       
      I have attached a reproducer, but to give an idea, please find below the kind of endpoints that now fails:

      @RestControllerpublic
      class DemoController {
      
          @PostMapping("/post")
          Mono<String> post(@RequestBody String text)  {
              return Mono.just(text).delayElement(Duration.ofMillis(10));
          }
      } 

       
      Notice we are unsure if Undertow 2.3.18.Final is expected to be used as it is tagged and published on Maven Central but does not appear on https://github.com/undertow-io/undertow/releases and is mentioned in the bug tracker as a non-released version. The Spring team would appreciate some guidance on which versions we are expected to leverage.

            [UNDERTOW-2512] Empty response body regression with request body and delayed response

            Thanks for your guidance, we have updated Spring adapter accordingly, see https://github.com/spring-projects/spring-framework/commit/35b452b458cd80ef6afd8c77dec0e0aa20dec755 related commit. If you see anything surprising, please let us know.

            Sébastien Deleuze (Inactive) added a comment - Thanks for your guidance, we have updated Spring adapter accordingly, see https://github.com/spring-projects/spring-framework/commit/35b452b458cd80ef6afd8c77dec0e0aa20dec755 related commit. If you see anything surprising, please let us know.

            sdeleuze Hey. Sure, I left my last comment hidden as I was editing it while I was looking into whole thing, partially cause I wanted to check how it worked before.
            Im going to uncover previous comment so you can catch up.

            Bottom line is that undertow directly support either in IO handling of whole request or as a dispatched task in executor.
            For GET method it looks like a bug on undertow side, which allows to set certain flag on terminated request.
            In case of POST/PUT methods, Spring properly read body(in undertow IO Thread) and than invoke Mono.delayElement , which essentially handle over context to timer thread/task and let IO thread go. Since request/exchange wasnt dispatched, its terminated right after Mono.delay return down the call stack.
            AFAIR, you will have to either use BlockingHandler or dispatch it yourself via: https://github.com/undertow-io/undertow/blob/main/core/src/main/java/io/undertow/server/HttpServerExchange.java#L833 / https://undertow.io/undertow-docs/undertow-docs-2.0.0/undertow-handler-guide.html#blocking-io ( I think, since you can provide executor which will run on your terms).
            Though Im not sure if it can be done after reading initial client input or prior.

            Its not stated directly, but if you look at HttpHandler.java#L22 The request handler must eventually either call another handler or end the exchange. - though it does not cover blocking, it hints that once handler return to undertow, undertow expects request "to be done" - either finished or dispatched. If you want to discuss further, reach out zulip

            Bartosz Baranowski added a comment - sdeleuze Hey. Sure, I left my last comment hidden as I was editing it while I was looking into whole thing, partially cause I wanted to check how it worked before. Im going to uncover previous comment so you can catch up. Bottom line is that undertow directly support either in IO handling of whole request or as a dispatched task in executor. For GET method it looks like a bug on undertow side, which allows to set certain flag on terminated request. In case of POST/PUT methods, Spring properly read body(in undertow IO Thread) and than invoke Mono.delayElement , which essentially handle over context to timer thread/task and let IO thread go. Since request/exchange wasnt dispatched, its terminated right after Mono.delay return down the call stack. AFAIR, you will have to either use BlockingHandler or dispatch it yourself via: https://github.com/undertow-io/undertow/blob/main/core/src/main/java/io/undertow/server/HttpServerExchange.java#L833 / https://undertow.io/undertow-docs/undertow-docs-2.0.0/undertow-handler-guide.html#blocking-io ( I think, since you can provide executor which will run on your terms). Though Im not sure if it can be done after reading initial client input or prior. Its not stated directly, but if you look at HttpHandler.java#L22 The request handler must eventually either call another handler or end the exchange. - though it does not cover blocking, it hints that once handler return to undertow, undertow expects request "to be done" - either finished or dispatched. If you want to discuss further, reach out zulip

            Happy to refine our Undertow adapter if it needs to be, but we would need more specific guidance as after reviewing it, we do not see what is currently wrong, and since it has been working flawlessly for years before with Undertow 2.3.17.Final and previous versions. We want to return from the handleRequest method, and continue handling asynchronously after exiting the callstack, do you spot anything we should do differently with Undertow API/SPI? Thanks in advance for your feedback.

            Sébastien Deleuze (Inactive) added a comment - - edited Happy to refine our Undertow adapter if it needs to be, but we would need more specific guidance as after reviewing it, we do not see what is currently wrong, and since it has been working flawlessly for years before with Undertow 2.3.17.Final and previous versions. We want to return from the handleRequest method, and continue handling asynchronously after exiting the callstack, do you spot anything we should do differently with Undertow API/SPI? Thanks in advance for your feedback.

            Bartosz Baranowski added a comment - - edited

            Making this private for now.

            Not sure yet how it worked before, but initial assumption is correct. Its bad integration.

            GET works because of possible bug on our side. What happens is GET gets its request terminated and than SPRING register its listener(UndertowServerHttpRequest$RequestBodyPublisher.registerListenersorg.springframework.http.server.reactive.UndertowServerHttpRequest$RequestBodyPublisher.registerListeners(UndertowServerHttpRequest.java:154)), which set "FLAG_SHOULD_RESUME_READS" on request that was terminated( This is likely a bug). Since there is no body, this flag is never reset after reading body, hence, in Connectors.executeRootHandlers  Connectors.java#L397is true and exchange is not ended, just falls into runResumeReadWrite(); DESPITE handing over dangling request to another, non IO/non executor thread, since endExchange();  wasnt called, HttpServerExchange#getResponseChannel wasnt called as well, so its possible to send response, even from stray thread. Thats exactly what happens.

            When it comes to POST/PUT its a bit different, because there is body, which is read in IO thread. So FLAG_SHOULD_RESUME_READS is reset:

            at io.undertow.server.protocol.http.HttpTransferEncoding$1.handleEvent(HttpTransferEncoding.java:179)
                    at io.undertow.server.protocol.http.HttpTransferEncoding$1.handleEvent(HttpTransferEncoding.java:172)
                    at io.undertow.conduits.FixedLengthStreamSourceConduit.invokeFinishListener(FixedLengthStreamSourceConduit.java:378)
                    at io.undertow.conduits.FixedLengthStreamSourceConduit.read(FixedLengthStreamSourceConduit.java:239)

            .
            This causes Connectors.executeRootHandlers to invoke endExchange, which will fetch responseChannel, send empty 200. Once timer wakes up in SPRING, it will try to send previously read body by invoking getResponseChannel, however, since it has:

             

            if (responseChannel != null) {
                        return null;
                    }
            

            SPRING will get null ref and terminate processing.

            So there are two things at least:
            1. improve terminated request handling( not allow reads?)
            2. possibly send/differentiate if possible not finished requests, rather than sending 200 by default?

            Bartosz Baranowski added a comment - - edited Making this private for now. Not sure yet how it worked before, but initial assumption is correct. Its bad integration. GET works because of possible bug on our side. What happens is GET gets its request terminated and than SPRING register its listener(UndertowServerHttpRequest$RequestBodyPublisher.registerListenersorg.springframework.http.server.reactive.UndertowServerHttpRequest$RequestBodyPublisher.registerListeners(UndertowServerHttpRequest.java:154)), which set "FLAG_SHOULD_RESUME_READS" on request that was terminated( This is likely a bug). Since there is no body, this flag is never reset after reading body, hence, in Connectors.executeRootHandlers   Connectors.java#L397 is true and exchange is not ended, just falls into runResumeReadWrite(); DESPITE handing over dangling request to another, non IO/non executor thread, since endExchange();   wasnt called, HttpServerExchange#getResponseChannel wasnt called as well, so its possible to send response, even from stray thread. Thats exactly what happens. When it comes to POST/PUT its a bit different, because there is body, which is read in IO thread. So FLAG_SHOULD_RESUME_READS is reset: at io.undertow.server.protocol.http.HttpTransferEncoding$1.handleEvent(HttpTransferEncoding.java:179)         at io.undertow.server.protocol.http.HttpTransferEncoding$1.handleEvent(HttpTransferEncoding.java:172)         at io.undertow.conduits.FixedLengthStreamSourceConduit.invokeFinishListener(FixedLengthStreamSourceConduit.java:378)         at io.undertow.conduits.FixedLengthStreamSourceConduit.read(FixedLengthStreamSourceConduit.java:239) . This causes Connectors.executeRootHandlers to invoke endExchange, which will fetch responseChannel, send empty 200. Once timer wakes up in SPRING, it will try to send previously read body by invoking getResponseChannel, however, since it has:   if (responseChannel != null) { return null; } SPRING will get null ref and terminate processing. So there are two things at least: 1. improve terminated request handling( not allow reads?) 2. possibly send/differentiate if possible not finished requests, rather than sending 200 by default?

            Bartosz Baranowski added a comment - - edited

            This is integration problem. In short spring tries to process request in async way without honoring API/SPI.

            Undertow generally has strict way of handling requests. Its either handled in IO thread or in blocking way in executor(dispatched): https://undertow.io/undertow-docs/undertow-docs-2.0.0/undertow-handler-guide.html#non-blocking-io .
            Spring starts in IO thread and upon reaching application code:

            return Mono.just(text).delayElement(Duration.ofMillis(10));

            What it essentially does it handles all undertow and spring mockups to Timer thread async to IO:

            public final Mono<T> delayElement(Duration delay, Scheduler timer)
            Unknown macro: { return onAssembly(new MonoDelayElement<>(this, delay.toNanos(), TimeUnit.NANOSECONDS, timer)); }

            https://github.com/reactor/reactor-core/blob/main/reactor-core/src/main/java/reactor/core/publisher/Mono.java#L2498

            , letting undertow IO thread to do its thing - in this case terminate request, in Connectors#executeRootHandler execution path:

            • L395#handler.handleRequest(exchange); - this is where spring app handler is invoked and hands over to Timer
            • L397#boolean resumed = exchange.isResumed(); - is false ( it will flip to true once above timer kicks in)
            • L398: false
            • L419: this is true, since resumed==false, this ends exchange async to timer thread

            Im still uncertain why it works for GET, but even if it works for it, does not change case for other methods - wrong integration/limitation of SPRING.

            sdeleuze  ^^

            EDIT: though there is still something fishy here.

            Bartosz Baranowski added a comment - - edited This is integration problem. In short spring tries to process request in async way without honoring API/SPI. Undertow generally has strict way of handling requests. Its either handled in IO thread or in blocking way in executor(dispatched): https://undertow.io/undertow-docs/undertow-docs-2.0.0/undertow-handler-guide.html#non-blocking-io . Spring starts in IO thread and upon reaching application code: return Mono.just(text).delayElement(Duration.ofMillis(10)); What it essentially does it handles all undertow and spring mockups to Timer thread async to IO: public final Mono<T> delayElement(Duration delay, Scheduler timer) Unknown macro: { return onAssembly(new MonoDelayElement<>(this, delay.toNanos(), TimeUnit.NANOSECONDS, timer)); } https://github.com/reactor/reactor-core/blob/main/reactor-core/src/main/java/reactor/core/publisher/Mono.java#L2498 , letting undertow IO thread to do its thing - in this case terminate request, in Connectors#executeRootHandler execution path: L395 #handler.handleRequest(exchange); - this is where spring app handler is invoked and hands over to Timer L397 #boolean resumed = exchange.isResumed(); - is false ( it will flip to true once above timer kicks in) L398 : false L419 : this is true, since resumed==false, this ends exchange async to timer thread Im still uncertain why it works for GET, but even if it works for it, does not change case for other methods - wrong integration/limitation of SPRING. sdeleuze   ^^ EDIT: though there is still something fishy here.

            The UNDERTOW-2436 seems to be at fault. The thing is Im unable so far to reproduce with delay/async etc in any way, with standalone undertow and for some reason IDE cant trigger breakpoint in libraries, only app/gradle code itself will do that, so its hard to debug.

            Bartosz Baranowski added a comment - The UNDERTOW-2436 seems to be at fault. The thing is Im unable so far to reproduce with delay/async etc in any way, with standalone undertow and for some reason IDE cant trigger breakpoint in libraries, only app/gradle code itself will do that, so its hard to debug.

            If that can help, I think the issue only happens when we delay writing the body, and the regression happened between 2.3.17 and 2.3.18. Maybe when there is a delay for writing the body, the headers are written too early with 2.3.18 hence the Content-Length: 0 sent other the wire? I saw some flushing related changes between 2.3.17 and 2.3.18, could it be related?

            Sébastien Deleuze (Inactive) added a comment - If that can help, I think the issue only happens when we delay writing the body, and the regression happened between 2.3.17 and 2.3.18. Maybe when there is a delay for writing the body, the headers are written too early with 2.3.18 hence the Content-Length: 0 sent other the wire? I saw some flushing related changes between 2.3.17 and 2.3.18, could it be related?

            spring/reactive create layers upon layers and mockup of undertow classes, which interact with undertow. So far Im unable to pinpoint where this happens but the thing is - both in mockups Iv seen and in OG undertow HttpServerExchange proper valuse are set - ie Content-Length: 54 ( which is value in test). The thin is that over wire response is sent with Content-Length: 0. Explicitly set value.

            Bartosz Baranowski added a comment - spring/reactive create layers upon layers and mockup of undertow classes, which interact with undertow. So far Im unable to pinpoint where this happens but the thing is - both in mockups Iv seen and in OG undertow HttpServerExchange proper valuse are set - ie Content-Length: 54 ( which is value in test). The thin is that over wire response is sent with Content-Length: 0. Explicitly set value.

            Downgrading. So far Im unable to reproduce with standalone, so this might be integration bug in 3rd parties.

            Bartosz Baranowski added a comment - Downgrading. So far Im unable to reproduce with standalone, so this might be integration bug in 3rd parties.

            bump priority based on description for now.

            Bartosz Baranowski added a comment - bump priority based on description for now.

              rhn-cservice-bbaranow Bartosz Baranowski
              sdeleuze Sébastien Deleuze (Inactive)
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Created:
                Updated:
                Resolved: