Incorrect err (nil) returned incase of timeout in httpu

[upperwal]

Problem: err returned in the following code

https://github.com/huin/goupnp/blob/167b9766e52022410644dfc64e8aadd57ee36858/httpu/httpu.go#L130-L132

points to ResolveUDPAddr err object.

https://github.com/huin/goupnp/blob/167b9766e52022410644dfc64e8aadd57ee36858/httpu/httpu.go#L83-L84

Scope of ReadFrom err is only in the for loop. Due to which if ReadFrom throws timeout error, for loop breaks but the error returned is not the one generated by ReadFrom instead it is the one generated by ResolveUDPAddr hence nil.

https://github.com/huin/goupnp/blob/167b9766e52022410644dfc64e8aadd57ee36858/httpu/httpu.go#L105-L112

[huin]

I'm not sure if the original code is actually incorrect. Can you explain more a case where the code does the wrong thing?

I think my intent on writing this code was that the timeout is not an error, but rather an indication that the function should stop waiting for further responses.

[upperwal]

Do you intent to send the previous err (error from ResolveUDPAddr) incase of timeout in the following code?

https://github.com/huin/goupnp/blob/167b9766e52022410644dfc64e8aadd57ee36858/httpu/httpu.go#L130-L132

Use case with timeout: Calling DiscoverDevices and it timed out. Output: DiscoverDevices returns (MaybeRootDevice = [], err = nil) Expected Output: DiscoverDevices should return (MaybeRootDevice = [], err = error("timeout"))

So if the current implementation is correct how are upstream function calls knowing about timeout from n, _, err := httpu.conn.ReadFrom(responseBytes)

SSDPRawSearch and all the downstream function calls are returning err = nil in case of timeout. Although responses is an empty array. Is this how it should work?

https://github.com/huin/goupnp/blob/167b9766e52022410644dfc64e8aadd57ee36858/goupnp.go#L60-L67

[huin]

I think the code I wrote was a bit misleading - the final line in HTTPUClient.Do should return responses, nil.

Note that the for loop is unconditional, and only breaks upon timeout - the timeout means that we've collected all results that we expect - it's not an error condition. If responses is empty, then that means that no HTTPU responses were received within the given timeframe, which may legitimately mean that there are no devices to respond to it. Or maybe there's some other condition preventing them from responding.

[huin]

I've committed 1395d1447324cbea88d249fbfcfd70ea878fdfca to clarify the code a little.

[upperwal]

Ok. If that's the intention then it's fine. Will be closing the issue and the corresponding PR.

Thanks.

[huin]

No problem :)