Problem/Motivation

Discovered in #2807263-7: Impossible to write unit tests involving Vocabulary, because TAXONOMY_HIERARCHY_(DISABLED|SINGLE|MULTIPLE) are defined in taxonomy.module instead of VocabularyInterface:

Coincidentally, this revealed that ResourceResponse still contains the data it must serialize! And so, whenever (Dynamic) Page Cache retrieves its cache items, the cache system unserializes the response, and so it must also unserialize ResourceResponse::$responseData.

This was the case ever since #1834288: RESTfully request an entity with JSON-LD serialized response, where RequestHandler calls setContent() on a ResourceResponse. This now lives at \Drupal\rest\RequestHandler::renderResponse(), but the basic principle is still the same. And that's the thing: the response will always have its response body set, there's no need for it to retain the object that was serialized into the response body.

Proposed resolution

Move the existing logic to take a ResourceResponse and generate a response body from RequestHandler (a controller) to ResourceResponseSubscriber (a KernelEvents::RESPONSE event subscriber).

This issue is not criticism on the REST module. It's just moving code to a response event subscriber, which is "the Symfony HttpKernel thing to do", i.e. it's how this sort of response manipulation is intended to work. Because this code was written early in the D8 cycle, this code was never updated to do this. So this patch does that.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Status: Active » Needs review
FileSize
786 bytes
Before
Requests per second:    90.57 [#/sec] (mean)
Time per request:       11.042 [ms] (mean)
Time per request:       11.042 [ms] (mean, across all concurrent requests)
Transfer rate:          70.14 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:    10   11   0.8     11      22
Waiting:       10   11   0.8     11      22
Total:         10   11   0.8     11      22
After
Requests per second:    144.27 [#/sec] (mean)
Time per request:       6.931 [ms] (mean)
Time per request:       6.931 [ms] (mean, across all concurrent requests)
Transfer rate:          111.73 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:     6    7   0.5      7      10
Waiting:        6    7   0.5      7      10
Total:          6    7   0.5      7      10
Delta
60% more requests per second. 60% of median response time. 55% lower max response time.
Wim Leers’s picture

Title: ResourceResponse::$responseData is serialized, but is never used: huge performance penalty » ResourceResponse::$responseData is serialized, but is never used: unserializing it on every Page Cache hit is a huge performance penalty
neclimdul’s picture

Super nice numbers. What happens when someone calls getResponseData() on an unserialized result though?

Wim Leers’s picture

That won't work. But why would code do that? Once the response has been determined, there's no need for that data to linger.

What's really happening here, is that RequestHandler::renderResponse() behaves as a response subscriber. It should transform ResourceResponse to a (Cacheable)Response object, so that Page Cache & Dynamic Page Cache cannot (un)serialize this data unnecessarily.

Wim Leers’s picture

Note that this technically breaks BC, because code may be extending RequestHandler (see #2718545: Refactor \Drupal\rest\RequestHandler::handle() into smaller methods). But as of right now, the jsonapi contrib module doesn't actually rely on overriding renderResponse(): http://cgit.drupalcode.org/jsonapi/tree/src/RequestHandler.php?id=id=2de....

Plus, it's very very likely to be the only contrib module, and it's targeted for core inclusion, so the risk in refactoring this code away is absolutely minimal.

Here's that alternative implementation. (Same performance numbers, because effectively the same data ends up in Page Cache.)

Wim Leers’s picture

Status: Needs review » Needs work

The last submitted patch, 6: 2807501-6.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
14.95 KB
1.01 KB

I forgot to migrate the status code. Easy fix. I should've run tests locally, then I would've caught this.

Status: Needs review » Needs work

The last submitted patch, 9: 2807501-9.patch, failed testing.

dawehner’s picture

  1. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -0,0 +1,210 @@
    +    $format = $this->getResponseFormat($this->routeMatch, $request);
    

    Given we are in a subscriber and not inside the routing layer any longer, I~m wondering whether using \Drupal\Core\Routing\RouteMatch::createFromRequest would be safer here.

  2. +++ b/core/modules/rest/src/RequestHandler.php
    @@ -129,118 +129,13 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
         return $response;
       }
    

    I'm wondering whether having some direct documentation here pointing towards the subscriber would make it clearer what is going on

Wim Leers’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
37.08 KB
23.76 KB

That assertion I included in the patch in #6 was well-intended, but it's sadly wrong. Removed that.

Also, I moved a bunch of code from RequestHandler to ResourceResponseSubscriber, but then did not update the kernel test coverage of RequestHandlerTest to move that to a ResourceResponseSubscriberTest. Did that.

Finally, I want to clarify that this issue is not criticism on the REST module. It's just moving code to a response event subscriber, which is "the Symfony HttpKernel thing to do", i.e. it's how this sort of response manipulation is intended to work. Because this code was written early in the D8 cycle, this code was never updated to do this. So this patch does that.

In dong so, I converted it from a Kernel test to a Unit test.

(Note that locally I'm running into bugs with run-tests.sh failing on PHPUnit tests with lots of @dataProvider test cases. I sure hope that testbot won't have the same problem… because with the phpunit test runner, it works just fine. The mere act of inserting the results into the database is what fails, because the column length is exceeded.)

Wim Leers’s picture

#11:

  1. This logic already exists in HEAD. It's just moved from the controller to a response event subscriber.
  2. There already is an @see on the handle() method's docblock. Happy to add it in the code too if you want. But this is nothing special, really. The same already happens for HTML responses, AJAX responses, and so on.

Status: Needs review » Needs work

The last submitted patch, 12: 2807501-11.patch, failed testing.

tstoeckler’s picture

This patch makes a lot of sense. I learned a lot from just reading it, thanks!

Some notes:

  1. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -0,0 +1,209 @@
    +    $response = $this->renderResponseBody($request, $response, $this->serializer, $format);
    ...
    +  protected function renderResponseBody(Request $request, ResourceResponseInterface $response, SerializerInterface $serializer, $format) {
    ...
    +    return $response;
    

    Minor, but seems unnecessary to return the response, when it's being passed in and modified.

    Without looking at the docs I would guess the return value of a method called renderResponseBody() to be a $response_body, not the $response. So not returning anything would be clearer in my opinion. I realize, that that's arguable, though, so feel free to leave as is, as well.

  2. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -0,0 +1,209 @@
    +  protected function flattenResponse(ResourceResponseInterface $response) {
    +    $final_response = ($response instanceof CacheableResponseInterface) ? new CacheableResponse() : new Response();
    +    $final_response->setContent($response->getContent());
    +    $final_response->setStatusCode($response->getStatusCode());
    +    $final_response->headers->add($response->headers->all());
    +    if ($final_response instanceof CacheableResponseInterface) {
    +      $final_response->addCacheableDependency($response->getCacheableMetadata());
    +    }
    +    return $final_response;
    +  }
    

    This is pretty nifty! Nice work.

    I looked at Symfony's Response and besides the stuff that is being taken care of here, there is the following state in a response object: version (the HTTP version), statusText, and charset.

    I can't actually think of those ever being very useful (maybe statusText, but I think we should take those over as well, just for completeness' sake.

  3. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
    @@ -0,0 +1,334 @@
    + * @group restkak
    

    :-)

  4. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
    @@ -0,0 +1,334 @@
    +
    +
    

    Double newline

  5. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
    @@ -0,0 +1,334 @@
    +      $handler_response = new ResourceResponse($method !== 'DELETE' ? ['REST' => 'Drupal'] : NULL);
    +      $this->assertInstanceOf(ResourceResponseInterface::class, $handler_response);
    +      $this->assertInstanceOf(CacheableResponseInterface::class, $handler_response);
    

    Am I missing or are these assertions pretty pointless?

  6. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
    @@ -0,0 +1,334 @@
    +      $this->assertInstanceOf(CacheableResponseInterface::class, $handler_response);
    

    Seems there is no test coverage for uncacheable responses?

    Not sure how hard that would be to integrate, though.

  7. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
    @@ -0,0 +1,334 @@
    +  protected function getFunctioningResourceResponseSubscriber(RouteMatchInterface $route_match) {
    

    Minor, but in my opinion the class would read more fluently if this were the last method.

  8. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
    @@ -0,0 +1,334 @@
    +    $renderer = $this->prophesize(RendererInterface::class);
    +    $renderer->executeInRenderContext(Argument::type(RenderContext::class), Argument::type('callable'))
    +      ->will(function ($args) {
    +        $callable = $args[1];
    +        return $callable();
    +      });
    

    Wow

  9. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
    @@ -0,0 +1,334 @@
    +/**
    + * Stub class where we can prophesize methods.
    + */
    +class StubRequestHandlerResourcePlugin extends ResourceBase {
    +
    +  function get() {}
    +  function post() {}
    +  function patch() {}
    +  function delete() {}
    +
    +}
    

    Unused here, I think.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
39.37 KB
8.65 KB
  1. Interesting feedback! I think you're right. Changed :)
  2. I actually copied this pattern from elsewhere, but yes, agreed! Done. Except statusText, because that is set automatically based on the status code, see \Symfony\Component\HttpFoundation\Response::setStatusCode().
  3. Oops :P You can read German, and for this word it's similar to Dutch, so … shit! Oh wait…
  4. Fixed.
  5. The intent is that the response object implements both of those interfaces, otherwise ResourceResponseSubscriber would not run certain bits of code. Then further down we assert that the returned response no longer implements one of the interfaces. So while it indeed is not super helpful test coverage itself, it makes the overall test coverage more clear IMO.
  6. This is great criticism. Note that this is just migrating existing test coverage from \Drupal\Tests\rest\Kernel\RequestHandlerTest. But I of course agree that it would be better to have proper full unit test coverage while we're touching this code anyway. So, added that.
  7. +1. Doing that in the next reroll because it makes the interdiff much harder to read otherwise.
  8. :P
  9. Hah, you're right! Removed.
Wim Leers’s picture

FileSize
39.37 KB
2.49 KB

And this then addresses #15.7.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Thank you, the fixes look great. Also looked through the patch again, and - again - found it very readable and a great cleanup. I'm general not an expert on the request processing system, but this A) just makes sense all around B) @dawehner and @neclimdul looked at it and didn't find it too offensive and C) is mostly moving code around, so I'm going to be a tad bold and RTBC this one.

Wim Leers’s picture

Thanks for the review and praise!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. Nice to see the performance improvements this patch brings and the evidence on the issue :)
  2. +++ b/core/modules/rest/rest.services.yml
    @@ -28,3 +28,8 @@ services:
    +  resource_response.subscriber:
    

    I think we should prefix the service name with rest. - whilst it is not a standard - it should be and definitely will make this addition less likely to clash.

  3. FILE: ...drupal/core/modules/rest/tests/src/Kernel/RequestHandlerTest.php
    ----------------------------------------------------------------------
       5 | WARNING | [x] Unused use statement
      13 | WARNING | [x] Unused use statement
      14 | WARNING | [x] Unused use statement
    

    There are now some unused uses in this test.

dawehner’s picture

  1. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -0,0 +1,206 @@
    +/**
    + * Response subscriber to handle resource responses.
    + */
    +class ResourceResponseSubscriber implements EventSubscriberInterface {
    

    i wonder whether we could describe better what this class is doing

  2. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -0,0 +1,206 @@
    +        if (!$context->isEmpty()) {
    +          $response->addCacheableDependency($context->pop());
    +        }
    +      }
    +      else {
    +        $output = $serializer->serialize($data, $format);
    +      }
    

    I'm wondering whether we could put the if into the if($context->isEmpty()) bit, so we keep just one $serializer->serialize() call

  3. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -0,0 +1,206 @@
    +    $final_response = ($response instanceof CacheableResponseInterface) ? new CacheableResponse() : new Response();
    +    $final_response->setContent($response->getContent());
    +    $final_response->setStatusCode($response->getStatusCode());
    +    $final_response->setProtocolVersion($response->getProtocolVersion());
    +    $final_response->setCharset($response->getCharset());
    +    $final_response->headers->add($response->headers->all());
    +    if ($final_response instanceof CacheableResponseInterface) {
    +      $final_response->addCacheableDependency($response->getCacheableMetadata());
    

    Is it just me, or is this kinda sad? We mostly copy an existing response, right? I'm a bit confused about the function name.

  4. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -0,0 +1,206 @@
    +    $events[KernelEvents::RESPONSE][] = ['onRespond'];
    

    nitpick: We could use onResponse

  5. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
    @@ -0,0 +1,371 @@
    +      // The default data for \Drupal\rest\ResourceResponse.
    +      [NULL, ''],
    +      [''],
    +      ['string'],
    +      ['Complex \ string $%^&@ with unicode ΑΒΓΔΕΖΗΘΙΚΛΜΝΞΟΣὨ'],
    +      [[]],
    +      [['test']],
    +      [['test' => 'foobar']],
    +      [TRUE],
    +      [FALSE],
    +      // @todo Not supported. https://www.drupal.org/node/2427811
    +      // [new \stdClass()],
    +      // [(object) ['test' => 'foobar']],
    

    Did you considered to use named data provider, but yeah, meh

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
40.15 KB
9.6 KB

#20:

  1. Done. Though I wanna call out that the reason I named it like I did before, is that we have html_response.subscriber and ajax_response.subscriber in core.services.yml. Still, I agree this is better.
  2. Done!

#21:

  1. This is consistent with \Drupal\Core\EventSubscriber\HtmlResponseSubscriber and \Drupal\Core\EventSubscriber\AjaxResponseSubscriber. But, improved it since you asked.
  2. This code was just being moved. But, improved it since you asked.
  3. I'm not sure I'd call this sad. We're fixing a design flaw in the REST module's ResourceResponse that's been there since the very beginning: we're ensuring that the response we send (to the end user, but hence also the the decorating middlewares) no longer contains the arbitrarily complex PHP object that it can contain in ResourceResponse::$responseData. The interface nor the implementation allow us to remove that data/object. So, we have no choice but to clone everything of an existing response except for this object. The removal of this object can be considered flattening, hence the name.
    Added some docs to clarify this.
  4. Sure, done!
  5. This code was just being moved. But, improved it since you asked.
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Done. Though I wanna call out that the reason I named it like I did before, is that we have html_response.subscriber and ajax_response.subscriber in core.services.yml. Still, I agree this is better.

Well, if we don't start now we never start.

This code was just being moved. But, improved it since you asked.

Mh sure, well, I try to improve things :)

alexpott’s picture

+++ b/core/modules/rest/src/RequestHandler.php
@@ -129,118 +129,13 @@ public function handle(RouteMatchInterface $route_match, Request $request) {
-  protected function getResponseFormat(RouteMatchInterface $route_match, Request $request) {
...
-  protected function renderResponse(Request $request, ResourceResponseInterface $response, SerializerInterface $serializer, $format, RestResourceConfigInterface $resource_config) {

I'm pondering about the BC implications of this change. The only usage in core is \Drupal\rest\Plugin\ResourceBase::getBaseRoute() as the controller class. We've already said that controllers are internal (see https://www.drupal.org/core/d8-bc-policy#controllers). So I think this change is allowed.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0f3f279 and pushed to 8.3.x. Thanks!

  • alexpott committed 0f3f279 on 8.3.x
    Issue #2807501 by Wim Leers, dawehner, tstoeckler: ResourceResponse::$...
Wim Leers’s picture

#23: yep! I'm just explaining the rationale behind the current code. I did agree those were all improvements, hence I made all those changes :)

Wim Leers’s picture

BTW, this triggered me to do more research. This made anonymous REST requests/responses 50% faster. But what about authenticated ones? For that, see #2827797: ResourceResponse(Subscriber) + Dynamic Page Cache: making authenticated ResourceResponses significantly faster.

Wim Leers’s picture

Also, I think a 60% performance/scalability boost (see #2) for anonymous REST requests is worth a mention in the 8.3.0 release notes?

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.