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

Catch IndexError #55

Merged
merged 1 commit into from
Oct 21, 2021
Merged

Catch IndexError #55

merged 1 commit into from
Oct 21, 2021

Conversation

niceking
Copy link
Contributor

@niceking niceking commented Oct 20, 2021

On occasion we're seeing the analytics gem hard fail (i.e. makes the tests fail) with IndexError. It's not great. Let's catch the error and trigger our reconnect flow. Alternate approach to #54.

The thing I'm not sure about: if an IndexError is thrown in other parts of the application that includes this gem, will it get caught here???

@niceking niceking requested a review from a team October 20, 2021 23:14
@linear
Copy link

linear bot commented Oct 20, 2021

PIE-71 rspec-buildkite-analytics sometimes fails with IndexError

We saw this bug happen in a few builds:

https://buildkite.com/buildkite/buildkite/builds/37449#d2e67d74-8a1d-4d74-8802-01cb65755abb

https://buildkite.com/buildkite/buildkite/builds/37511#1a95eb50-8bed-4568-b220-a9a9fede9835

It seems that this is caused by non-thread safe use of the underlying SSL Socket library, which we are technically culpable of 😅, but also it shouldn't bubble up as an array error! I raised this as an issue

ruby/openssl#452

Which has spawned this fix, which has now been merged ruby/openssl#453

So it shouldn't manifest as an IndexError anymore…but I am not sure what it will manifest as! I think let's just monitor and see, and hopefully this error crops up again, and will manifest in a different way, or manifest in a way that we're already dealing with.

View original card in Trello

Copy link
Member

@blaknite blaknite left a comment

Choose a reason for hiding this comment

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

As much as I dislike catching such a non-descript error as this one, I think it's the only simple path forward. One caveat though...can you put a ticket in Linear for us to reassess a proper fix?

The thing I'm not sure about: if an IndexError is thrown in other parts of the application that includes this gem, will it get caught here???

This will only catch IndexError raised inside the call stack of these methods. So yes, we risk catching any and all index errors that occur in anything called inside initialize and transmit but nothing outside (eg test/application code).

In normal operation this isn't the kind of error you'd expect to see so I think it's ok but with the caveat raised above.

Side note: I think this has less risk than pinning OpenSSL dependency to a point release as in the related ticket.

@niceking
Copy link
Contributor Author

@niceking niceking merged commit 45ba4a6 into main Oct 21, 2021
This was referenced Oct 21, 2021
@niceking niceking deleted the PIE-71/catch-indexerror branch November 4, 2021 23:20
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