open-coap / java-coap

CoAP Java library
Apache License 2.0
18 stars 5 forks source link

should `responseTimeout` be mandatory ? #54

Closed sbernard31 closed 1 year ago

sbernard31 commented 1 year ago

Since recently (maybe release v6.12.0), there is a new responseTimeout concept. This is a great addition.

Currently this is mandatory but If someone want to manage this kind of timeout on its own currently there is no way to deactivate this. A workaround could be to set a very long timeout but maybe, it could make sense to be able to deactivate this ?

sbernard31 commented 1 year ago

The question is maybe why someone would like to handle response timeout on its own ?

I don't know if there is good reason. :thinking:
In my case, this is maybe a not so good reason... I already implement it and when I integrated v6.12.0 and I wanted to go back to previous behavior without going deeper in this new feature.

Maybe instead I should use the new feature correctly. Here some question to be sure it match my needs :

  1. This responseTimeout can be set at CoapServer level but also at CoapRequest level using RESPONSE_TIMEOUT in Transport Context ?
  2. How does is work with BlockTransfer ?
  3. Is there a way to know if this is a ResponseTimeout or a CoAP Reliability Timeout ? I think both failed in a same way raising a CoapTimeoutException ?
sbernard31 commented 1 year ago

Not directly linked but reading the code, I see that there is also a responseTimeout for notificaiton :

Service<SeparateResponse, Boolean> sendNotification = new NotificationValidator()
              .andThen(new BlockWiseNotificationFilter(capabilities()))
              .andThen(new ResponseTimeoutFilter<>(timer, req -> req.getTransContext().getOrDefault(RESPONSE_TIMEOUT, responseTimeout)))
              .andThen(Filter.of(CoapPacket::from, CoapPacket::isAck))
              .andThenMap(midSupplier::update)
              .andThen(retransmissionFilter)
              .andThen(piggybackedExchangeFilter)
              .then(sender);

Not sure to get the meaning of this ? :thinking:

szysas commented 1 year ago

I assume that by saying 'mandatory' you mean it always have some value, and if not specified in CoapServerBuilder it will use default value (2 minutes). It's purpose is to timeout:

Are you suggesting that there are use cases where there is no need to have a timeout guard for some transactions?

  1. This responseTimeout can be set at CoapServer level but also at CoapRequest level using RESPONSE_TIMEOUT in Transport Context ?

That's true. Setting it in CoapRequest will overwrite CoapServer one.

  1. How does is work with BlockTransfer ?

Timer starts on first block and is not restarted on each block.

  1. Is there a way to know if this is a ResponseTimeout or a CoAP Reliability Timeout ? I think both failed in a same way raising a CoapTimeoutException ?

That's true. We could easily add cause for timeout exception if you see a need.

szysas commented 1 year ago

Not directly linked but reading the code, I see that there is also a responseTimeout for notificaiton :

It is used to timeout outgoing observations.

sbernard31 commented 1 year ago

I assume that by saying 'mandatory' you mean it always have some value, and if not specified in CoapServerBuilder it will use default value (2 minutes)

Yep, that what I meant

sbernard31 commented 1 year ago

Are you suggesting that there are use cases where there is no need to have a timeout guard for some transactions?

Not really, I was more thinking about cases where java-coapuser implements its own way to handle coap response timeout. (which is my case for now).

For me, maybe it's better to consider to reuse coap response timeout from java-coap (let's see if this is possible) In a more general perspective, I guess being able to deactivate could make sense but I haven't strong argument (as maybe for me there is another way)

sbernard31 commented 1 year ago

That's true. Setting it in CoapRequest will overwrite CoapServer one. Timer starts on first block and is not restarted on each block.

That sounds good to me.

That's true. We could easily add cause for timeout exception if you see a need.

To have exactly same behavior than what I have currently I need something like this.

sbernard31 commented 1 year ago

It is used to timeout outgoing observations.

I'm not sure I get it.
This is about sending Notification right ?
But a notification is either a CON or a NON ?
For NON there is nothing to timeout ?
For CON no need to response timeout as there is reliability timeout out (based on ACK + retransmission)
Or maybe this is about Notification with blockwise ?

szysas commented 1 year ago

Yes, you're right, for sending notifications there is no use of it. Not even for blockwise transfer as we only notify first block and the rest has to be requested by recipient (so it's separate transaction).

In general, in your use case maybe better to set it to something big and handle timeouts in leshan. Just remember to call cancel on response's CompletableFuture.

szysas commented 1 year ago

Reopen if you think there is still something more to discuss.

sbernard31 commented 1 year ago

In general, in your use case maybe better to set it to something big and handle timeouts in leshan. Just remember to call cancel on response's CompletableFuture.

I do not like so much the idea about having big value instead of deactivating but I can live with it for now. (I may come back to you on this later when I will be in a more polishing phase)

Thx for you help :pray: