8000 [RFC][WebProfilerBundle] Slow kernel.terminate can prevent Toolbar from loading · Issue #25849 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[RFC][WebProfilerBundle] Slow kernel.terminate can prevent Toolbar from loading #25849

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
JoeKre opened this issue Jan 19, 2018 · 8 comments
Closed

[RFC][WebProfilerBundle] Slow kernel.terminate can prevent Toolbar from loading #25849

JoeKre opened this issue Jan 19, 2018 · 8 comments
Labels
Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) WebProfilerBundle

Comments

@JoeKre
Copy link
Contributor
JoeKre commented Jan 19, 2018
Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? yes
Symfony version >= 3.4

I would like to get your opinion concerning the WebDebugToolbar and potentially long running kernel.terminate events when sending spooled mails which "breaks" the toolbar

The scenario can be described as follows:

When using the SwiftMailerBundle with any type of spooling active i.e. spool: { type: memory } the actual sending will be done through Symfony\Bundle\SwiftmailerBundle\EventListener\EmailSenderListener before the kernel is actually terminated.

This will lead to the EmailSenderListener blocking the kernel termination until the response from your email provider is fully processed.
Until the kernel is fully terminated the profile for the given token cannot be loaded.
While this is happening the toolbar might already try to load the profile data through its ajax call.

Based on the performance of the email provider or any potential SwiftMailer plugins it can happen that the WebDebugToolbar will quit after the 5 tries to load the profile and show the error instead.

Two possible fixes for this problem could be to either:

  • increase the number of retries for the ajax call (possibly through a configurable value)

or

  • add a small delay before sending the 404 if the profile could not be loaded and create more time for kernel.terminate to finish running.

Simplified example in Symfony\Bundle\WebProfilerBundle\Controller\ProfilerController::toolbarAction

if (!$profile = $this->profiler->loadProfile($token)) {
    sleep(1);
    return new Response('', 404, array('Content-Type' => 'text/html'));
}

Is there any need to fix this "problem" which is mildly annoying although it's not actually a bug in the profiler?
If someone should fix it, or rather give the toolbar more leeway to load the profile data, which way would be a preferred one?

kind regards
Joe
@javiereguiluz javiereguiluz added Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Jan 22, 2018
@nicolas-grekas
Copy link
Member

I'm not sure there is anything we should do on Symfony's side. Making background tasks on kernel.terminate should be replaced by a proper job queue when this situation is reached instead.

@JoeKre
Copy link
Contributor Author
JoeKre commented Jan 22, 2018

In general i agree with you however in this case we are not really talking about making some big tasks on kernel.terminate.
It's more like a combination of use the MailerBundle with spooling enabled and sending a single mail and a slow mail provider slowing down the sending of spooled mails.

Moving the email sending to a job or setup an infrastructure using exim or anything like that would of course work around the problem but seems like a bit too much for a simple workaround.

@stof
Copy link
Member
stof commented Jan 22, 2018

Well, there is 3 options for that IMO:

  • keeping the current behavior
  • stop profiling the work happening during kernel.terminate, allowing to save the profile earlier (which basically reverts to the behavior of earlier 2.x versions of Symfony)
  • add more delay before retrying the loading of the toolbar in case of 404, to give it more chance to succeed after kernel.terminate is done.

As we already retry the loading of the toolbar, the third solution might be a viable option, by only delaying retries (so that the loading is not delayed too much for most pages not using kernel.terminate)

@stof
Copy link
Member
stof commented Jan 22, 2018

note that delaying the retry should not be done by blocking the server during 1s before sending the 404 for the previous try. It should rather be done by making the JS wait (in a non-blocking way) before the retry.

@JoeKre
Copy link
Contributor Author
JoeKre commented Jan 23, 2018

@stof

guess that could be easily done by small changes to the js without any side effects:

toolbar_js.html.twig@124:
{ maxTries: 5, timeoutDelay: 1000 }

and base_js.html.twig@30+:

options.maxTries = options.maxTries || 0;
options.timeoutDelay = options.timeoutDelay || 500;
xhr.open(options.method || 'GET', url, true);
xhr.setRequestHeader('X-Requested-With', 'XMLHttpRequest');
xhr.onreadystatechange = function(state) {
    if (4 !== xhr.readyState) {
        return null;
    }

    if (xhr.status == 404 && options.maxTries > 1) {
        setTimeout(function(){
            options.maxTries--;
            request(url, onSuccess, onError, payload, options);
         }, options.timeoutDelay);

         return null;
    }

@nicolas-grekas
Copy link
Member

@JoeKre would you mind sending a PR?

@JoeKre JoeKre mentioned this issue Jan 30, 2018
Closed
@JoeKre
Copy link
Contributor Author
JoeKre commented Jan 30, 2018

First time for everything i guess :)

I hope i didn't completely screw something up with the process for contributing.

javiereguiluz added a commit that referenced this issue Jan 30, 2018
@javiereguiluz
minor #25967 [WebProfilerBundle] Increase retry delays between toolba…
b9bb577 
…rAction ajax calls (JoeKre)

This PR was submitted for the master branch but it was squashed and merged into the 2.7 branch instead (closes #25967).

Discussion
----------

[WebProfilerBundle] Increase retry delays between toolbarAction ajax calls

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes/no
| Fixed tickets | #25849
| License       | MIT
| Doc PR        | -

Commits
-------

3288fb0 [WebProfilerBundle] Increase retry delays between toolbarAction ajax calls
@javiereguiluz
Copy link
Member

Fixed by #25967.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) WebProfilerBundle
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@javiereguiluz @nicolas-grekas @stof @xabbuh @JoeKre
0