ruby / resolv

A thread-aware DNS resolver library written in Ruby
Other
38 stars 29 forks source link

Fix confusion of received response message #9

Closed rhenium closed 3 years ago

rhenium commented 3 years ago

This is a follow up for commit 33fb966197f1 ("Remove sender/message_id pair after response received in resolv", 2020-09-11).

As the senders instance variable is also used for tracking transaction ID allocation, simply removing an entry without releasing the ID would eventually deplete the ID space and cause Resolv::DNS.allocate_request_id to hang.

It seems the intention of the code was to check that the received DNS message is actually the response for the question made within the method earlier. Let's have it actually does so.

[Bug #12838] https://bugs.ruby-lang.org/issues/12838 [Bug #17748] https://bugs.ruby-lang.org/issues/17748

CvX commented 3 years ago

Good work! 👏

Should a regression test be added for this change? (e.g. assert(Resolv::DNS::RequestID.empty?) after calling getresource())

jsdalton commented 3 years ago

I filed an issue yesterday since we recently ran into exactly this. See https://github.com/ruby/resolv/issues/11 (I missed this PR when I opened the issue.)

I would +1 the suggestion for a unit test though, especially as the PR here effectively reverts the production code change from 33fb966 while leaving the test introduced in that commit in place. This indicates to me: