ResponseTimeout is not configurable on per message basis

[kulak]

ResponseTimeout const control message response timeout and it is set to 2 seconds.

While 2 seconds timeout is ok for 1st message, when message is re transmitted the timeout value needs to be increased. Thus, we need to be able to supply custom ResponseTimeout with the request data as it appears to be a higher level function in this API design.

const (
    // ResponseTimeout is the amount of time to wait for a
    // response.
    ResponseTimeout = time.Second * 2
)

// Receive a message.
func Receive(l *net.UDPConn, buf []byte) (Message, error) {
    l.SetReadDeadline(time.Now().Add(ResponseTimeout))

    nr, _, err := l.ReadFromUDP(buf)
    if err != nil {
        return Message{}, err
    }
    return parseMessage(buf[:nr])
}

[drasko]

Also, I can notice that in the example/obsclient we are trying simple Receive() which timeouts observation after 2 sec: https://github.com/dustin/go-coap/blob/master/example/obsclient/obsclient.go#L37

This timeout must be modifiable, as we might decide to wait on receive for a long time in a separate goroutine (we observe some value and we do not know in which moment the value will come - after 2 seconds or after 10 minutes...)

[kulak]

Hi,

I generally don't contribute to github, so I am not sure how it all works.

I have my own fork where I have addressed the issue, because our solution requires a control over response timeout at client level.

Here is a single commit where I address the issue. I have added a test case to verify that I have addressed timeout:

https://github.com/Kulak/go-coap/commit/4ac9ea7ce27f73d47de90e6c58ac7a1ea2b8ff81

The solution I chose is backwards compatible, but it introduces new variant of Dial function. I added a variable to track timeout parameter. I have renamed original ResponseTimeout to be DefaultResponseTimeout, because I wanted to make sure I've got all use cases and because its meaning is to be the default.

I am personally not sure if it would've been better to request for timeout as an argument to Send call. Compatibility and simplicity of API won over and I added response timeout to Connection structure instead.

If there is an interest in merge, I will request a pull for merge.

[dustin]

I'd really like to figure out how to get context.Context plumbed through all this. This codebase predates it, so maybe this would warrant a V2, but context solves all of the timeout problems and communication of timeouts as well as explicit cancellations in a clear and consistent (but incompatible) way. I think it's the right way moving forward. Comments?

On Tue, Jun 6, 2017 at 4:08 PM Kulak notifications@github.com wrote:

Hi,

I generally don't contribute to github, so I am not sure how it all works.

I have my own fork where I have addressed the issue, because our solution requires a control over response timeout at client level.

Here is a single commit where I address the issue. I have added a test case to verify that I have addressed timeout.

The solution I chose is backwards compatible, but it introduces new variant of Dial function. I added a variable to track timeout parameter. I have renamed original ResponseTimeout to be DefaultResponseTimeout, because I wanted to make sure I've got all use cases and because its meaning is to be the default.

I am personally not sure if it would've been better to request for timeout as an argument to Send call. Compatibility and simplicity of API won over and I added response timeout to Connection structure instead.

If there is an interest in merge, I will request a pull for merge.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/dustin/go-coap/issues/43#issuecomment-306642805, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAG88Dca7cg-3s6tBRlUVLcEr4u47mpks5sBdvtgaJpZM4NA95G .

[kulak]

Context is a very good approach. I did not even think about it as I don't usually use it because of similar reason of code base older than context. I've used context concept in .NET apps and it is a great solution to many issues with logical operation context.

[bgentry]

@dustin I definitely agree that utilizing context.Context is the way to go here. You can model it after what net/http does and make the context a part of the coap.Message. Handlers would be expected to respect ctx.Done() as they are in HTTP these days.