Fix flush thread crashing (AttributeError) on a real server HTTP 500 - #41
Open
jaideeppyne wants to merge 1 commit into
Open
Fix flush thread crashing (AttributeError) on a real server HTTP 500#41jaideeppyne wants to merge 1 commit into
jaideeppyne wants to merge 1 commit into
Conversation
`FlushWorker.step` checked `getattr(response, "exception") != None` without a default. On a network error the uploader returns a `Fake500` that carries an `exception` attribute, but a genuine server HTTP 500 is a real `requests.Response`, which has no such attribute. So on a real 500 the `getattr` raised `AttributeError`, which propagated out of `step()`/`run()` and killed the background flush thread — every subsequent log was then queued but never sent for the life of the process. Pass a default (`getattr(response, "exception", None) is not None`) so a real Response skips the branch instead of crashing, while the network-error Fake500 path still logs as before. Adds two regression tests: a real `requests.Response` 500 must not crash the worker (fails on the old code with the exact AttributeError from logtail#25), and the Fake500 network-error path must still log. Note: the existing tests use `MagicMock(status_code=500)`, whose auto-vivified `.exception` masks the bug, so the new test uses a real Response. Fixes logtail#25.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #25.
FlushWorker.stepdecides whether to log an upload failure with:getattr(response, "exception")is called without a default. The two thingsstepcan receive are asymmetric:Uploader.__call__catchesrequests.RequestExceptionand returnsFake500(e)— which has an.exceptionattribute.session.post(...)returns a realrequests.Response— which has no.exceptionattribute.So on a real 500,
status_code == 500isTrue, thengetattr(response, "exception")raisesAttributeError. That propagates out ofstep()→run(), so the background flush thread dies. Every subsequent log is enqueued but never sent for the rest of the process — silent log loss. This matches the reporter's "happens occasionally" (real 500s are intermittent).Fix
A real
Responsenow yieldsNoneand skips the branch; the network-errorFake500path still logs its exception as before. (Also switched!= None→is not Noneto match idiom.)Tests
Two regression tests in
tests/test_flusher.py:test_step_does_not_crash_on_real_server_500— returns a realrequests.Responsewithstatus_code = 500;step()must not raise. This fails on the current code with the exactAttributeError: 'Response' object has no attribute 'exception'from Flusher fails to log: AttributeError: 'Response' object has no attribute 'exception' #25.test_step_still_logs_network_error_fake500— theFake500network-error path still logs.mock.MagicMock(status_code=500), andMagicMockauto-vivifies.exception, which is why they never caught this — the new test deliberately uses a realResponse.Full
tests/test_flusher.pysuite passes (11 tests).