Skip to content
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.

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

Convert frozen middleware error into a warning in the Rails integration #611

Merged
merged 1 commit into from
Jul 27, 2020

Conversation

imjoehaines
Copy link
Contributor

@imjoehaines imjoehaines commented Jul 23, 2020

Goal

When running RSpec in a Rails app which has an error in the boot process, Bugsnag will appear to be throwing errors when injecting middleware because the Rails app's middleware stack is frozen (see #534). This isn't actually caused by Bugsnag; the error in the Rails boot process freezes the middleware and when RSpec attempts to run the subsequent files it's never unfrozen. This makes frozen middleware errors inevitable, unless nothing adds middleware, but if we run first then we'll trigger the error. The stacktrace does point out the actual problem, but only in the very first error, which can get buried under the frozen errors that happen for each test file

This comment explains what happens in depth: thoughtbot/factory_bot_rails#303 (comment)

Tests

I've manually tested this catches the frozen error that us trying to inject middleware causes when running RSpec in a broken Rails app. The setup for this is super convoluted and isn't really worth automating — it's possible in a maze runner scenario but you'd need a rails app specifically setup to trigger this and some RSpec tests to run and parse the RSpec output for stack frames with 'bugsnag' in them

Linked issues

Fixes #534

Review

For the submitter, initial self-review:

  • Commented on code changes inline explain the reasoning behind the approach
  • Reviewed the test cases added for completeness and possible points for discussion
  • A changelog entry was added for the goal of this pull request
  • Check the scope of the changeset - is everything in the diff required for the pull request?
  • This pull request is ready for:
    • Initial review of the intended approach, not yet feature complete
    • Structural review of the classes, functions, and properties modified
    • Final review

For the pull request reviewer(s), this changeset has been reviewed for:

  • Consistency across platforms for structures or concepts added or modified
  • Consistency between the changeset and the goal stated above
  • Internal consistency with the rest of the library - is there any overlap between existing interfaces and any which have been added?
  • Usage friction - is the proposed change in usage cumbersome or complicated?
  • Performance and complexity - are there any cases of unexpected O(n^3) when iterating, recursing, flat mapping, etc?
  • Concurrency concerns - if components are accessed asynchronously, what issues will arise
  • Thoroughness of added tests and any missing edge cases
  • Idiomatic use of the language

@imjoehaines imjoehaines marked this pull request as draft July 23, 2020 09:51
@imjoehaines imjoehaines force-pushed the supress-frozen-middleware-error branch 2 times, most recently from 885be37 to 4a0fd98 Compare July 23, 2020 16:14
@imjoehaines imjoehaines marked this pull request as ready for review July 24, 2020 08:43
Copy link
Contributor

@kattrali kattrali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers for figuring this one out! 🕵️

This should never happen in normal operation, but is possible when
running RSpec tests

See thoughtbot/factory_bot_rails#303 (comment)
and #534

Essentially this happens:
1. RSpec boots Rails
2. Rails boot process errors
3. Rails freezes middleware
4. RSpec moves on to the next test file
5. Bugsnag tries to add middleware but fails because it's frozen
6. The stacktrace blames Bugsnag for this test failure
7. goto 4

Supressing this error doesn't solve the problem as it's likely another
frozen error will happen in some other library/component, but we want
to avoid people thinking this is caused by Bugsnag. The stacktrace
does point out the actual problem, but only in the very first error,
which can get buried under the frozen errors that happen for each
test file
@imjoehaines imjoehaines force-pushed the supress-frozen-middleware-error branch from 4a0fd98 to a530de7 Compare July 27, 2020 12:54
@imjoehaines imjoehaines merged commit 1d5e201 into next Jul 27, 2020
@imjoehaines imjoehaines deleted the supress-frozen-middleware-error branch August 12, 2020 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants