Incorrect err (nil) returned incase of timeout in httpu

[upperwal]

Problem:
err returned in the following code

goupnp/httpu/httpu.go

Lines 130 to 132 in 167b976

	}
	return responses, err
	}

points to ResolveUDPAddr err object.

goupnp/httpu/httpu.go

Lines 83 to 84 in 167b976

	destAddr, err := net.ResolveUDPAddr("udp", req.Host)
	if err != nil {

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.

goupnp/httpu/httpu.go

Lines 105 to 112 in 167b976

	for {
	// 2048 bytes should be sufficient for most networks.
	n, _, err := httpu.conn.ReadFrom(responseBytes)
	if err != nil {
	if err, ok := err.(net.Error); ok {
	if err.Timeout() {
	break
	}

[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?

goupnp/httpu/httpu.go

Lines 130 to 132 in 167b976

	}
	return responses, err
	}

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?

goupnp/goupnp.go

Lines 60 to 67 in 167b976

	func DiscoverDevices(searchTarget string) ([]MaybeRootDevice, error) {
	httpu, err := httpu.NewHTTPUClient()
	if err != nil {
	return nil, err
	}
	defer httpu.Close()
	responses, err := ssdp.SSDPRawSearch(httpu, string(searchTarget), 2, 3)
	if err != nil {

[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 1395d14 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 :)