Skip to content

excluded_http_codes not working as intended for http errors thrown inside the callback of a StreamedResponse #57902

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
mathieuimbert opened this issue Aug 1, 2024 · 5 comments
Closed

excluded_http_codes not working as intended for http errors thrown inside the callback of a StreamedResponse #57902

mathieuimbert opened this issue Aug 1, 2024 · 5 comments
Labels
Bug MonologBridge Status: Needs Review Status: Waiting feedback

Comments

@mathieuimbert
Copy link

Symfony version(s) affected

6.4.10

Description

  • Symfony: 6.4.10
  • Monolog: 3.7.0

I stumbled on this issue after upgrading from Symfony 5 to 6. When a HttpException is thrown inside the callback of a StreamedResponse, it is ignored by excluded_http_codes and logged anyway.

This happens because $this->requestStack->getMainRequest() returns null in https://github.com/symfony/symfony/blob/6.4/src/Symfony/Bridge/Monolog/Handler/FingersCrossed/HttpCodeActivationStrategy.php#L53

This is a consequence of the stack->pop() done in https://github.com/symfony/symfony/blob/6.4/src/Symfony/Component/HttpKernel/HttpKernel.php#L103 as soon as the callback ran.

How to reproduce

  1. Setup a fingers_crossed monolog handler with excluded_http_codes
main:
        type: fingers_crossed
        action_level: warning
        handler: json
        excluded_http_codes: [400, 401, 403, 404, 405, 429]
        buffer_size: 50
    json:
        type:  rotating_file
        path: "%kernel.logs_dir%/%kernel.environment%.log"
        formatter: 'monolog.formatter.json'
        level: debug
        max_files: 2
  1. Create a simple controller that returns a StreamedResponse:
<?php

namespace App\Controller;

use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\HttpFoundation\StreamedResponse;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
use Symfony\Component\Routing\Attribute\Route;

class HomeController extends AbstractController
{
    #[Route('/home', name: 'app_home')]
    public function index(): StreamedResponse
    {
        $response = new StreamedResponse();
        $response->setCallback(function () {
            throw new NotFoundHttpException('This is a test exception');
        });
        return $response;
    }
}

  1. Go to /home and notice that the 404 error is logged

  2. Move throw new NotFoundHttpException('This is a test exception'); before the setCallback() function and notice that the 404 error is not logged (as expected)

Possible Solution

No response

Additional Context

No response
@nicolas-grekas
Copy link
Member

Wasn't this fixed by #51396
Can you please put your reproducer in a github repository one could clone to reproduce easily?

@xabbuh
Copy link
Member

xabbuh commented Sep 13, 2024

Let’s close here meanwhile. We can reopen if it turns out that it wasn’t fixed by the linked PR.

@xabbuh xabbuh closed this as completed Sep 13, 2024
@mathieuimbert
Copy link
Author

@nicolas-grekas Sorry for the delay, here's a repo that reproduces the issue: https://github.com/mathieuimbert/symfony-streamed-response-issue.

@nicolas-grekas nicolas-grekas mentioned this issue Sep 17, 2024
Merged
@nicolas-grekas
Copy link
Member

Thanks for the reproducer, this will be fixed by #58289

@mathieuimbert
Copy link
Author

@nicolas-grekas Indeed, thank you 🥳 Let me know if I can help with your PR.

nicolas-grekas added a commit that referenced this issue Sep 17, 2024
@nicolas-grekas
bug #58289 [HttpKernel] Skip logging uncaught exceptions in `ErrorHan…
ad3f44e 
…dler`, assume `$kernel->terminateWithException()` will do it (nicolas-grekas)

This PR was merged into the 5.4 branch.

Discussion
----------

[HttpKernel] Skip logging uncaught exceptions in `ErrorHandler`, assume `$kernel->terminateWithException()` will do it

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #57902
| License       | MIT

When uncaught exceptions are handled by `ErrorHandler::handleException()`, that method logs them, then calls a callback to let the app know about the exception itself.

This causes two issues:
1. that log happens without context, which leads to #57902
2. logs are duplicated: one by ErrorHandler, twice by the callback

In this PR, I propose to consider that if terminateWithException is configured as a callback, we assume that the app will handle the logging on its own.

Tested in practice, this works nicely.

Adding a test case is quite complex as all this involves many pieces and global state, so I skipped adding one.

Commits
-------

3b87c32 [HttpKernel] Skip logging uncaught exceptions in ErrorHandler, assume $kernel->terminateWithException() will do it
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug MonologBridge Status: Needs Review Status: Waiting feedback
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@nicolas-grekas @xabbuh @mathieuimbert @carsonbot