Fix some cases where we could crash trying to iterate over nil
#2493
Conversation
dljsjr
requested review from
NoahCornell,
cjswedes,
gharveymn,
hcarter-775,
nickolas-deboom,
varzac and
z-michel
|
Channel deleted. |
Test Results 71 files 457 suites 0s ⏱️ Results for commit 8e3bc94. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 8e3bc94 |
dljsjr
force-pushed
the
fix/sonos-nil-iteration-crash
branch
from
b2cedb3 to
93689d5
Compare
dljsjr
force-pushed
the
fix/sonos-nil-iteration-crash
branch
from
93689d5 to
8e3bc94
Compare
There was a problem hiding this comment.
Seems like all pairs/ipairs were updated with protections against nil input. Im fine with this, but if were doing it for fields we know should never be nil when accessed, it could potentially be hiding future errors that we would only discover with user feedback rather than driver crashes.
| --- time that `receive_m_search_response` will return results with a timeout. | ||
| function _ssdp_mt:multicast_m_search() | ||
| for term, _ in pairs(self.search_terms) do | ||
| for term, _ in pairs(self.search_terms or {}) do |
There was a problem hiding this comment.
Seems like we may want to crash here if this is nil, since it would mean potentially not discovering devices when we should be.
Yeah, I went back and forth on this a bit. Here's my take on it: We've learned that crashes in LAN drivers like this can lead to unintended hot crash loops. While we have a variety of mitigations on place on the hub to limit the impact of these situations, they often still end up causing undue burden on the hub's resources. And we usually end up getting bug reports for this stuff anyway. That said there's definitely still value in the early canary of the driver crash metrics, so I could see undoing this in some places being valuable. |
3 participants
Check all that apply
Type of Change
Checklist
Description of Change
The recent refactor to reduce memory usage also performed a normalization of variable casing; all values that are "our" data structures use
snake_casenames, and all values that are "their" data structures (i.e. tables that are created directly from JSON or XML bodies) usecamelCase.Since we're no longer storing the REST response bodies as-is, but just a subset of them, there are a few places where variables that used to be
camelCaseare nowsnakeCase; we addressed all of these direct accesses, but we missed a few places where we created fallback values for iteratees that might benil.While fixing the above, we also add empty table fallbacks to almost all iteration sites to avoid this writ-large.
Summary of Completed Tests
Tested on my personal setup, however, I believe that the crash this fixes is only reproducible on S1 bonded sets/stereo pairs, which I don't have the personal inventory to test (my only bonded-set eligible speakers are also S2 only speakers).