CompressAndTruncate

[chantra]

Currently the Truncate function disables compression, unless the does not fit in size bytes.
E.g after calling msg.Truncate(1234), msg.Compress will be false when returning if its uncompressed length is < 1234. Even though the caller may have set msg.Compress to true explicitly before.

As much as I do understand the reasoning, I am not sure why this is forced on the user of the library.

What would be the right way to support both use cases? Would simply removing

dns/msg_truncate.go

Lines 41 to 42 in 35023fa

	// Don't waste effort compressing this message.
	dns.Compress = false

be OK?
But this would change the current behaviour though, on the other hand, if someone had set msg.Compress before hand, they are probably not replying what they expect to reply.

Should another function be created to allow to Truncate without side effect on the dns.Compress attribute?
As much as I understand the attempt to compress before truncating, I can also see people that would rather truncate and not compress...

What would you see as the best middle ground here?

[tmthrgd]

Compression doesn’t affect the semantics of the message in anyway so I don’t see the problem. If someone really always wants compression, they can just set Compress to true unconditionally after calling Truncate.

As for truncating without compressing, that seems a bit pointless given the purpose is to fit as much of the message into the given space as possible and compressing helps with that.

I’m not sure any changes or API additions would be justified here.

[chantra]

If someone really always wants compression, they can just set Compress to true unconditionally after calling Truncate.

Sure one can do that, but the side-effect of the Truncate function is still counter-intuitive IMHO. If someone had set their intent to compress a message prior, why would calling Truncate change that? What are the benefits if the caller knowingly set it (given that it defaults to false)?

The current documentation also does not reflect this behaviour, it does tell that it checks if it fits without compression, then with compression.... but never that if it fits without compression, it will reset the compression attribute.

Actually, removing

dns/msg_truncate.go

Lines 41 to 42 in 35023fa

	// Don't waste effort compressing this message.
	dns.Compress = false

would match the documentation.

As for truncating without compressing, that seems a bit pointless given the purpose is to fit as much of the message into the given space as possible and compressing helps with that.

I do agree, I just said that I could see someone willing to trade retry over TCP vs walking and compressing, but I honestly don't have any interest in the support and this would probably complicate things more than anything.

[miekg]

Agreeing w/ Tom here. Actually rfc 1035 says something sane about this: Programs are free to avoid using pointers in messages they generate, although this will reduce datagram capacity, and may cause truncation. However all programs are required to understand arriving messages that contain pointers. (TIL)

[chantra]

I am not saying that compression is compulsory and that the server must compress. What I am saying is that Truncate has the side-effect of resetting Compress to false when it was previously set by the caller, which is counter-intuitive to me and goes against the caller original intent. Even more so that it is not documented.

If you strongly believe this is the right thing to do, then I suppose the fix would be to make it clear in the documentation that if the uncompress message fits within size, the compression attribute will be cleared and the caller will need to set it back if they wish to compress the message anyhow.

I can provide either patch.

[tmthrgd]

I don't think changing behaviour is really possible as it has been in the library as is for just under two years now, and it's easily worked around. I also implemented Truncate intentionally to behave similarly to, and replace, the Scrub method that was once part of coredns. You'll note that Scrub had the same clearing Compress behaviour.

I think documenting this behaviour is fine though, feel free to open a PR for that.

[chantra]

ok, I will cut a PR with some wording about the behaviour then.