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

dnsdist: Fix short IOs over TCP #7974

Merged
merged 4 commits into from
Jun 27, 2019

Conversation

rgacogne
Copy link
Member

@rgacogne rgacogne commented Jun 24, 2019

Short description

TCPIOHandler::tryRead() and TCPIOHandler::tryWrite() are supposed to update the position and read/write the remaining number of bytes based on the initial total number minus the position, so that the caller can just pass the exact same parameters on short reads/writes until it succeeds or an exception is raised, without having to care about updating the position and number of bytes to read/write.
Untested yet.

Supersedes #7971.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@rgacogne
Copy link
Member Author

Added some checks suggested by @omoerbeek and fixed the existing one, which compared the size of the buffer to the requested size, not the requested size minus the existing position.

@rgacogne rgacogne requested a review from omoerbeek June 24, 2019 12:34
@Habbie
Copy link
Member

Habbie commented Jun 24, 2019

Untested yet.

It fixes the problem in my test environment!

@rgacogne
Copy link
Member Author

I forgot to update my comment but I actually tested it in the meantime! I reproduced the issues with short reads and short writes with master, and confirmed that this PR fixed them.
I'll try to write a test for these cases, since it's already the second time I manage to introduce a bug in that exact spot.

@omoerbeek
Copy link
Member

I approved this, but now it looks to me like not all cases of tryRead were covered.

Copy link
Member

@omoerbeek omoerbeek left a comment

Choose a reason for hiding this comment

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

Seems a few calls to tryRead were missed when converting the 3rd arg. There are 4 calls here,
line 1045 got converted, 1067 looks right, but 936 and 946 need fixing.

@omoerbeek omoerbeek self-requested a review June 24, 2019 18:47
pdns/tcpiohandler.hh Outdated Show resolved Hide resolved
@rgacogne
Copy link
Member Author

Seems a few calls to tryRead were missed when converting the 3rd arg. There are 4 calls here,
line 1045 got converted, 1067 looks right, but 936 and 946 need fixing.

You are mistaking TCPIOHandler::tryRead() and a static function named tryRead(), which exists because we don't speak DoT toward backends atm and so have no need for the filters abstraction. I have a WIP branch unifying those two.

@omoerbeek
Copy link
Member

omoerbeek commented Jun 24, 2019

Seems a few calls to tryRead were missed when converting the 3rd arg. There are 4 calls here,
line 1045 got converted, 1067 looks right, but 936 and 946 need fixing.

You are mistaking TCPIOHandler::tryRead() and a static function named tryRead(), which exists because we don't speak DoT toward backends atm and so have no need for the filters abstraction. I have a WIP branch unifying those two.

Ah, right. Did not spot that and jumped to conclusions. Having two conventions for the third arg is confusing...

Copy link
Member

@omoerbeek omoerbeek left a comment

Choose a reason for hiding this comment

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

A second read

@rgacogne
Copy link
Member Author

Now with regression tests.

@rgacogne rgacogne merged commit 346c2d1 into PowerDNS:master Jun 27, 2019
@rgacogne rgacogne deleted the ddist-tcp-short-writes branch June 27, 2019 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants