Ensure the name cache is rolled back when the packet reaches maximum size #318
Conversation
This file contains 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
bdraco
force-pushed
the
fix_reset_on_packets
branch
2 times, most recently
from
809c3d8
to
6ed8c0a
Compare
|
Any easy way we can modify the test suite to verify this? |
|
Coverage increased (+0.01%) to 93.387% when pulling 6ed8c0aaeb74e2c817e48d34539f1c73da544e08 on bdraco:fix_reset_on_packets into 4da1612 on jstasiak:master. |
|
I'll mark this as a draft while I think of a way to write a test for it. It will probably need to wait to the weekend when I have some more time. Its been like this for a while so I don't think its on fire and needs attention right now. |
|
Definitely needs a test as I just got another case of it in the logs so I may not have fixed it or at least not all the instances of it. |
|
|
|
Looks like I need to dig in more |
|
Was able to verify its choking on a packet it is sending. |
|
I think something in the first packet is pointing to something in the second packet. |
I think I've got enough to replay the packet construction now |
|
Here is the DNSOutgoing repr that triggers the issue: |
|
And a second one to be sure |
|
@stephenrauch Any chance you can take a look? I've got a script that replicates the compression issue: https://github.com/bdraco/python-zeroconf/blob/choke_fix/choke.py |
|
I finally found the actual problem. When we rollback because the packet is too large we fail to remove the names so we create pointers to names that have been moved to the next packet. |
bdraco
force-pushed
the
fix_reset_on_packets
branch
from
6ed8c0a
to
0869f62
Compare
bdraco
changed the title
bdraco
changed the title
|
I'm going to take |
bdraco
force-pushed
the
fix_reset_on_packets
branch
4 times, most recently
from
b40eeeb
to
f88f738
Compare
|
Verified the no more |
|
Nice! I'll have a look at this soon. |
bdraco
force-pushed
the
fix_reset_on_packets
branch
from
f88f738
to
daae7dc
Compare
bdraco
force-pushed
the
fix_reset_on_packets
branch
5 times, most recently
from
8f2667a
to
a71fa51
Compare
|
CI looks happy now: https://travis-ci.org/github/jstasiak/python-zeroconf/builds/752058074 |
bdraco
force-pushed
the
fix_reset_on_packets
branch
from
a71fa51
to
5ca3c2b
Compare
|
I think I should take out the exact packet matching. As long as re-loading it with DNSIncoming doesn't throw we know its ok |
bdraco
force-pushed
the
fix_reset_on_packets
branch
from
5ca3c2b
to
bef4c0d
Compare
|
That's cleaner. I think this is ready to go. I just retested it |
This was referenced Dec 30, 2020
|
Running for 24 hours and no more errors in the log on the site that this was happening 👍 |
|
Still looking good. @jstasiak Any chance we can get a release with this before monday so we can get it into the next Home Assistant release on wednesday? Thank you |
|
@bdraco I'm sorry, didn't have time to look at this until now. The new test finishes successfully for me (Mac OS 10.14, CPython 3.9.0) without changes to the library code so while I trust the issue is fixed the test we have doesn't actually verify it. |
|
I'll roll the test back to a prior revision. I looks I was too aggressive in trying to reduce the size of the test case. |
…size If the packet was too large, it would be rolled back at the end of write_record. We need to remove the names that were added to the name cache (self.names) as well to avoid a case were we would create a pointer to a name that was rolled back. The size of the packet was incorrect at the end after the inserts because insert_short would increase self.size even though it was already accounted before. To resolve this insert_short_at_start was added which does not increase self.size. This did not cause an actual bug, however it sure made debugging this problem far more difficult. Additionally the size now inserted and then replaced when the actual size is known because it made debugging quite difficult since the size did not previously agree with the data.
bdraco
force-pushed
the
fix_reset_on_packets
branch
from
bef4c0d
to
f20e851
Compare
|
I was able to keep the test almost as-is and add this: Verified it passes on the new code and fails on the old code. |
|
@jstasiak retested after walking away for a minute and should be good to go. Thanks for validating the test. Sorry I missed it in the first place. |
Nice! I was afraid the binary dump of the DNS packets would be back (I could live with it, but it's better not to have it). I'll review (and hopefully release) soon. |
|
@bdraco Released as version 0.28.8. No worries about the test. :) |
|
Thank you! |
21 tasks
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.
Fixes #308
If the packet was too large, it would be rolled back at the end of
write_record.We need to remove the names that were added to the name cache (
self.names)as well to avoid a case were we would create a pointer to a name that was
rolled back.
The size of the packet was incorrect at the end after the inserts because
insert_shortwould increaseself.sizeeven though it was already accountedbefore. To resolve this
insert_short_at_startwas added which does notincrease
self.size. This did not cause an actual bug, however it suremade debugging this problem far more difficult.
Additionally the size now inserted and then replaced when the actual
size is known because it made debugging quite difficult since the size
did not previously agree with the data.