flaky test

[marten-seemann]

flaky test

[marten-seemann]

Looks like this test got flakier with the recent changes.

[betamos]

I just ran the race detector in my project and it caught the TTL issue immediately. In my case:

server, _ := zeroconf.Register(...) // Starts the server, which can read the ttl value at any point
server.TTL(1234) // Writes the ttl value without thread safety

So I'd upgrade this from "flaky test" to "actual bug" that will happen. We could:

1. Use atomic load/store on the ttl value. Note this still has a logical race condition between default ttl and user-provided.
2. Provide ttl in a new spread arg opts ... to Register (similar to Browse to avoid breaking API).

I'd prefer 2, WDYT? Accepting PRs?

[marten-seemann]

1. Use atomic load/store on the ttl value. Note this still has a logical race condition between default ttl and user-provided.

Agreed. This is ugly.

2. Provide ttl in a new spread arg opts ... to Register (similar to Browse to avoid breaking API).

Sounds good. Looking at the API, it would be nice to get rid of the []net.Interface as well (as we do for the client), but that would be breaking interface change, so let's not do that. Strictly speaking, adding an ...Option is also a breaking change, but we can probably just ignore that to avoid cutting v3 ;)

Is there any real use case for modifying the TTL after the fact? Otherwise, we can just deprecate that function.

Accepting PRs?

Yes! That would be highly appreciated!

[betamos]

Great! Sent a PR your way. Left the test itself alone for now, since it uses a harness method that I didn't want to fiddle with too much.

Is there any real use case for modifying the TTL after the fact? Otherwise, we can just deprecate that function.

I don't think so either. Something along the lines of "I'm going away soon, so please overwrite your previous TTL with a shorter one" perhaps, and if so also v3 material? Deprecated it in the PR.