Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#15 | 2794089-15.patch | 935 bytes | Wim Leers |
Comments
Comment #2
cilefen CreditAttribution: cilefen commentedComment #3
cilefen CreditAttribution: cilefen commentedComment #4
Wim LeersWhich 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.
Comment #5
yobottehg CreditAttribution: yobottehg commentedremoving the nginx tag because on one project using apache we have also this warnings.
I looked in the modules list and here it is:
The only modules i can think of which can cause this are redirect and r4032login which return a RedirectResponse
Comment #6
Wim LeersI know that the following are safe (or at least extremely unlikely to cause problems):
My biggest suspects:
Can you try uninstalling all of the other ones one-by-one to find the root cause?
Comment #7
yobottehg CreditAttribution: yobottehg commentedr4032login works like this:
The question is how should it work to not throw this warning so i can provide a patch?
Comment #8
Wim LeersThanks! :) 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.
Comment #9
Wim LeersNow reproduced. Working on determining the root cause.
Comment #10
Wim LeersThis line in
\Drupal\r4032login\EventSubscriber\R4032LoginSubscriber::onKernelException()
causes a session to be started:(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.
\Drupal\Core\Session\SessionManager::destroy()
is not capable of working in streamed responses. It does this, which fails: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 callSessionManager::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'ssetcookie()
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.
Comment #11
Fabianx CreditAttribution: Fabianx for Drupal Association commentedIndeed 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
Comment #13
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedI can confirm that this patch successfully fixes the problem for one of our sites.
Comment #14
Berdir@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 :)
Comment #15
Wim LeersI 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.Comment #16
Fabianx CreditAttribution: Fabianx for Drupal Association commentedRTBC, lets go!
Comment #17
Berdir+1
Comment #18
slashrsm CreditAttribution: slashrsm as a volunteer commented+1
Comment #20
catchCommitted/pushed to 8.3.x, leaving RTBC against 8.2.x for cherry-pick.
Comment #21
Wim LeersYay!
Would indeed be great to land in 8.2, because otherwise this bug will simply be reported again against 8.2.
Comment #22
catchMoving 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.
Comment #24
catch