I use the BigPipe module in a Drupal 8 instance with 15 authors and have some warnings in the Logs which i wanted to share. Perhaps someone has an idea whats going wrong.

Warning: Cannot modify header information - headers already sent by (output started at /opt/nginx/vhosts/xxx/htdocs/web/core/modules/big_pipe/src/Render/BigPipe.php:247) in Drupal\Core\Session\SessionManager->destroy() (Zeile 262 in /opt/nginx/vhosts/xxx/htdocs/web/core/lib/Drupal/Core/Session/SessionManager.php).

Everything is working, so it should be a minor thing.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

yobottehg created an issue. See original summary.

cilefen’s picture

Title: Warning in the Logs » Cannot modify header information - headers already sent by (output started at /opt/nginx/vhosts/xxx/htdocs/web/core/modules/big_pipe/src/Render/BigPipe.php:247) in Drupal\Core\Session\SessionManager->destroy()
cilefen’s picture

Title: Cannot modify header information - headers already sent by (output started at /opt/nginx/vhosts/xxx/htdocs/web/core/modules/big_pipe/src/Render/BigPipe.php:247) in Drupal\Core\Session\SessionManager->destroy() » Cannot modify header information - headers already sent by (output started at web/core/modules/big_pipe/src/Render/BigPipe.php:247) in Drupal\Core\Session\SessionManager->destroy()
Issue tags: +nginx
Wim Leers’s picture

Category: Bug report » Support request
Status: Active » Postponed (maintainer needs more info)

Which contrib modules have you installed? Can you reproduce the problem with vanilla Drupal 8 + BigPipe? It's very likely that this is caused by a contrib module that is explicitly sending HTTP headers, circumventing Symfony's abstraction.

yobottehg’s picture

Status: Postponed (maintainer needs more info) » Active
Issue tags: -nginx

removing the nginx tag because on one project using apache we have also this warnings.

I looked in the modules list and here it is:

  • ctools
  • default_content
  • devel
  • dropzonejs
  • editor_advanced_link
  • embed
  • entity_browser
  • entity_reference_revisions
  • file_browser
  • file_entity
  • imagemagick
  • linkit
  • mailsystem
  • pragraphs
  • pathauto
  • r4032login
  • rabbit_hole
  • redirect
  • swiftmailer
  • token
  • yamlform

The only modules i can think of which can cause this are redirect and r4032login which return a RedirectResponse

Wim Leers’s picture

Status: Active » Postponed (maintainer needs more info)

I know that the following are safe (or at least extremely unlikely to cause problems):

  • editor_advanced_link
  • embed
  • file_entity
  • linkit
  • pathauto
  • token

My biggest suspects:

  • devel
  • mailsystem
  • r4032login
  • rabbit_hole

Can you try uninstalling all of the other ones one-by-one to find the root cause?

yobottehg’s picture

Status: Postponed (maintainer needs more info) » Active
  • Okay so, devel was there but uninstalled, did not cause the problem.
  • mailsystem uninstall did not change anything.
  • rabbit_hole was also not the cause
  • r4032login uninstall has helped shut down the warnings

r4032login works like this:

  • Catch AccessDeniedHttpException in onKernelException in an EventSubscriber
  • Creates a RedirectResponse

The question is how should it work to not throw this warning so i can provide a patch?

Wim Leers’s picture

Title: Cannot modify header information - headers already sent by (output started at web/core/modules/big_pipe/src/Render/BigPipe.php:247) in Drupal\Core\Session\SessionManager->destroy() » BigPipe + r4032login module: "headers already sent" error
Assigned: Unassigned » Wim Leers

Thanks! :) Now I know where to look. I'll figure out a solution. I'll report back later today.

Note that this is the very first case of a contrib module not being compatible with BigPipe.

Wim Leers’s picture

Now reproduced. Working on determining the root cause.

Wim Leers’s picture

Title: BigPipe + r4032login module: "headers already sent" error » SessionManager::destroy() is incompatible with streamed responses: tries to send response header after headers have already been sent, causing watchdog to fill up with PHP warnings
Component: big_pipe.module » request processing system
Assigned: Wim Leers » Unassigned
Category: Support request » Bug report
Priority: Normal » Minor
Status: Active » Needs review
Issue tags: +BigPipe
FileSize
864 bytes

This line in \Drupal\r4032login\EventSubscriber\R4032LoginSubscriber::onKernelException() causes a session to be started:

drupal_set_message(Xss::filterAdmin($message), $message_type);

(This code runs only for anonymous users.) It's configurable, but it's enabled by default. Which means that by default, this module prevents caching by Page Cache/Varnish/…, which is pretty bad for performance. But it may be a good thing to do for usability.

  1. Note that if it would not do this, then BigPipe would not even be triggered, because BigPipe is only used for responses with sessions.
  2. But because it does this, a session is started, solely for showing the message. The response that is sent is a redirect response. This cannot show the message. The browser follows the redirect, and BigPipe sees there's a session, so it automatically turns the streamed response into a BigPipe response. When Drupal is finished rendering the messages, it removes them from the session. Drupal notices this and marks the session obsolete; it destroys it. It sadly is this act of destroying the session that fails, because \Drupal\Core\Session\SessionManager::destroy() is not capable of working in streamed responses. It does this, which fails:
    setcookie($session_name, '', REQUEST_TIME - 3600, $params['path'], $params['domain'], $params['secure'], $params['httponly']);
    

    It fails, because it causes a response header to be sent, but at this time it's too late to send a response header.

This is an actual bug. The problem is more on the Symfony/Drupal side than on the BigPipe side though: SessionManager::destroy() simply doesn't support streamed responses. One must call SessionManager::destroy() before sending anything. That's wrong.


The fix is hence quite simple: let SessionManager::destroy() only delete a cookie (which it btw does by using PHP's setcookie() rather than using Symfony's $response->headers->clearCookie()) when no headers were sent yet.

On the next request, Drupal will again notice that the session is obsolete, and it will again try to delete it, but this time it will do it before streaming a response (i.e. in the Session middleware), and so it'll be able to successfully close the session.


Marking as a "minor bug", because it doesn't cause any harm, it just is annoying because it causes the status log to fill up with annoying PHP warnings.

Fabianx’s picture

Indeed if that used the Symfony API function that was meant for that case, it would be not necessary to check for headers_sent() as Symfony does set cookies only when sending the actual Response:

https://github.com/symfony/http-foundation/blob/master/Response.php#L347

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

slashrsm’s picture

I can confirm that this patch successfully fixes the problem for one of our sites.

Berdir’s picture

Status: Needs review » Needs work

@slashrsm confirmed that this fixes the problem for our site, code and explanation does make sense to me. I read #11 also as a +1.

Tests would be rather complicated I guess.

Was about to set to RTBC.... but lets add a comment to that if to explain that in 1-2 sentences :)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
837 bytes
935 bytes

I kept the comment very short, because explaining just one potential edge case that is even BigPipe-specific seems wrong to do in \Drupal\Core\Session\SessionManager. It simply describes what is already a fact.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, lets go!

Berdir’s picture

+1

slashrsm’s picture

+1

  • catch committed 7e7de02 on 8.3.x
    Issue #2794089 by Wim Leers, yobottehg, slashrsm: SessionManager::...
catch’s picture

Committed/pushed to 8.3.x, leaving RTBC against 8.2.x for cherry-pick.

Wim Leers’s picture

Yay!

Would indeed be great to land in 8.2, because otherwise this bug will simply be reported again against 8.2.

catch’s picture

Status: Reviewed & tested by the community » Patch (to be ported)

Moving to 'to be ported' because it's currently impossible to distinguish between issues RTBC against 8.2.x that haven't been committed to any branch, vs. issues that have already been committed to 8.3.x and are waiting for 8.2.0 to land.

  • catch committed dc2798b on 8.2.x
    Issue #2794089 by Wim Leers, yobottehg, slashrsm: SessionManager::...
catch’s picture

Status: Patch (to be ported) » Fixed

Status: Fixed » Closed (fixed)

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