Allow users to cancel requests

[michaelbeaumont]

The reason for introducing this is the potentially unexpected behavior around reconnect attempts and timeouts. If the client disconnects and begins attempting to reconnect, it pauses the current request timeout (and will also therefore never timeout any additional pending requests).

I think it's important to be able to allow canceling or setting an absolute timeout for requests (here via context.WithTimeout).

Otherwise, the user has no way of reacting to the fact that their requests aren't making it and may never make it to the central system (and also making sure they don't then get sent once the server connection is reestablished, thus the check before the Write call).

WDYT?

[lorenzodonini]

Quick question regarding the timeout: do you expect a use-case, in which this would be useful? The TimeoutConfig should already set timeouts, so having a double timeout seems like overkill. Totally agreed for cancelling requests though.

I've actually been wanting to integrate contexts for a while. I also though we could potentially pass down the top-level callback through that context, which would save us a mutex and some management headaches in the ocpp16 module.

I'd love to test & integrate this, once it's ready to go.
If you need some help fixing the tests, just let me know and I can have a look.

[michaelbeaumont]

I think the TimeoutConfig just leads to the client disconnecting, leading to the scenario I mentioned where a request effectively might never time out.

[lorenzodonini]

I think the TimeoutConfig just leads to the client disconnecting, leading to the scenario I mentioned where a request effectively might never time out.

Not exactly. If the client is disconnected from the server, then the dispatcher is paused, as you correctly pointed out. This leads to requests not being canceled but staying in the queue until they are eventually delivered.

The TimeoutConfig is used for timing out response messages: if a response is never received within time N, even though there were no disconnects, then the onRequestTimeout callback is invoked. This goes back to the application with a GenericError and some timeout details. Btw this behavior is somewhat documented in the OCPP-J specification.

[lorenzodonini]

I stand corrected: the timeout policy on the dispatcher currently doesn't use the value from the TimeoutConfig. It uses a default 30 seconds timeout value, with the option of setting the timeout manually.

I had planned to have the dispatcher use the TimeoutConfig as a default value, but apparently never hooked it up. I also realize now, that it would be using a websocket-specific configuration on a higher layer, which is probably not ideal.

[michaelbeaumont]

I stand corrected: the timeout policy on the dispatcher currently doesn't use the value from the TimeoutConfig. It uses a default 30 seconds timeout value, with the option of setting the timeout manually.

I had planned to have the dispatcher use the TimeoutConfig as a default value, but apparently never hooked it up. I also realize now, that it would be using a websocket-specific configuration on a higher layer, which is probably not ideal.

In particular, the websocket TimeoutConfig operates at the websocket level, so the peer could theoretically keep pinging and never trigger that time out. The only per request timeout active is the one in the dispatcher as far as I can tell, and once it's paused, both a request waiting in the dispatcher queue and AFAICT an already dispatched request will never time out.

I think with this PR or some variation of it, the existing ClientDispatcher.timer could even be eliminated.

[lorenzodonini]

once it's paused, both a request waiting in the dispatcher queue and AFAICT an already dispatched request will never time out.

Correct. The idea was to give some support to charge points, that have poor/intermittent connection in the first place. Causing a timeout, while the connection is interrupted, seems very annoying (at least that's some feedback I had previously received). Willing to change this behavior ofc.

the existing ClientDispatcher.timer could even be eliminated.

That timer could indeed be replaced by the context. I would still want to have a default timeout value on the dispatcher, since requests should eventually timeout imho.

What if we added the context to the base Request and Response? It would require a small change in every message, but we would not have to break the current interface, we would maintain backwards-compatibility and setting default values would also be easy (the http package does the same).

Btw, don't get me wrong, I definitely want to integrate this PR somehow, just trying to find a good compromise for usability, while not disrupting the interface.

[michaelbeaumont]

What if we added the context to the base Request and Response? It would require a small change in every message, but we would not have to break the current interface, we would maintain backwards-compatibility and setting default values would also be easy (the http package does the same).

I personally like having the Request and Response be "pure structured data" and it seems more semantically accurate to provide a Context to Send because it belongs to the operation rather than data.
I could just add new SendCtx methods to avoid breaking the interface?

[michaelbeaumont]

once it's paused, both a request waiting in the dispatcher queue and AFAICT an already dispatched request will never time out.

Correct. The idea was to give some support to charge points, that have poor/intermittent connection in the first place. Causing a timeout, while the connection is interrupted, seems very annoying (at least that's some feedback I had previously received). Willing to change this behavior ofc.

I can definitely see that being useful. I suppose as a charge point in most cases, there's nothing to do except wait on the connection whereas for my use case, I can take different actions based on whether or not the client request has made it to the central system in some amount of time.