Read timeout accounts for total time to receive relevant packet

[rlisagor]

Original code simply set the timeout on the socket, which meant that it would apply to each packet received. However, packets may not be relevant. This is an especially significant problem on the AF_PACKET socket as it would see all packets on the interface.

[d2g]

Sorry for the slow reply I feel really ignorant now, especially as I appreciate your efforts and you're 100% correct.

Quick headlines:

• Is this a breaking change? people are more likely to see timeouts?
  • Should the error passed back be of our own type? How does the standard lib handle timeouts? (Is there a quick win to increase quality)

[stapelberg]

Is this a breaking change? people are more likely to see timeouts?

No, the opposite: people are less likely to run into excessive timeouts (because previously, the timeout would start over from scratch with every (non-relevant) packet received).

Should the error passed back be of our own type? How does the standard lib handle timeouts? (Is there a quick win to increase quality)

Yes, I think so. I also think that we should check in SendPacket whether errors are syscall.EAGAIN and return a friendlier error message. Something like:

if errno, ok := err.(syscall.Errno); ok && errno == syscall.EAGAIN {
    return fmt.Errorf("no DHCP packet received within %v", c.timeout)
}

I can pick up this PR and update the error handling. Just let me know if you agree with my assessment above :)

[d2g]

No, the opposite: people are less likely to run into excessive timeouts (because previously, the timeout would start over from scratch with every (non-relevant) packet received).

In the case where this timeout has been set to a low value because irreverent packets are keeping the timeout from occurring. If we modify this then timeout are more likely to occur.

However, with the objective to move this forward I'm happy with your approach. In the V2 branch I've added an error type and included the above change already (thoughts).

[stapelberg]

In the case where this timeout has been set to a low value because irreverent packets are keeping the timeout from occurring. If we modify this then timeout are more likely to occur.

That’s fair, but it seems unlikely to me that this would be a surprising change to users. I would imagine that people would file a bug before blindly setting the timeout to a ridiculously small value (and then relying on that keep working!) :)

However, with the objective to move this forward I'm happy with your approach. In the V2 branch I've added an error type and included the above change already (thoughts).

Ah, I didn’t see that the change was already included in the V2 branch.

What are your plans for the V2 branch? If it’s not right around the corner, would it make sense to merge this change also for the master branch?

[d2g]

relying on that keep working

Agreed.

What are your plans for the V2 branch? If it’s not right around the corner, would it make sense to merge this change also for the master branch?

I'm not actively using the library hence the recent new tests to try and identify the current status. I wrote this for a POC some time ago and although I'm not using it I still would like to resolve the outstanding issues. I'm looking to gain confidence in the V2 branch prior to forcing it out. I'd appreciate if you could assist with this?

On this issue I agree we should make this change if you're happy with the error handling.

[rlisagor]

Apologies for dropping the ball on this one, I completely missed the initial reply and only noticed it now that the thread has been resumed nearly a year later. Thanks for following up, all.

[stapelberg]

@rlisagor No worries. Do you want to update the error handling as discussed, or would you prefer if I took care of that?

@d2g Sure, I can take a look at the V2 branch and see if it works for me. I’ll let you know.

[rlisagor]

I won't have the time to do this in the near future, so if you're up for it, please do.

[stapelberg]

done: #20 (i.e., this PR can now be closed)