Open michaelbeaumont opened 3 years ago
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.
I think the TimeoutConfig
just leads to the client disconnecting, leading to the scenario I mentioned where a request effectively might never time out.
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.
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.
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.
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.
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?
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.
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?