eclipse-californium / californium

CoAP/DTLS Java Implementation
https://www.eclipse.org/californium/
Other
725 stars 361 forks source link

setReadyToSent call several times with transparent block mode. #818

Open sbernard31 opened 5 years ago

sbernard31 commented 5 years ago

When you are using transparent block mode using block2, all MessageObserver of "parent" request will be used for each child/block requests.

So events like setReadyToSent could be raised several time, which is a bit counterintuitive.

I didn't remember why we are doing this and maybe there is a good reason to keep this like this. (I agree that touching transparent block mode is a bit freaky)

But maybe we could at least pass the request as parameter to be able to know which request matches which event ?

(here is a leshan bug relative to this : https://github.com/eclipse/leshan/issues/623)

boaks commented 5 years ago

The outer observer handles cancel, also for inner cancels. Sometimes it's also useful to track the "blockwise" by counting onSent(). So, if a observer is intended to be only called once, in short term I would use AtomicBoolean to do so (e.g. BaseMatcher.ObservationObserverAdapter.onRemove()).

We sure could also pass in the Message, that would in some cases allow to re-use the same observer, but I'm not sure, if the "Observer" should then be renamed. And in the case above; i'm not sure, if this would be a easier approach then the AtomicBoolean.

Anyway, I stuck at scandium, so, if you want, just go for changing it.

sbernard31 commented 5 years ago

I will first go for a fix in Leshan (ensuring that I do not schedule several time the cleaning task) I will see If I found time to add message to observer method. (if this makes sense)

boaks commented 5 years ago

Split the "forwarding" from PR #848, it should be possible to "fix" this also with a new "forward" approach.

boaks commented 5 years ago

My main concern about changing that is, it requires a lot of testing and so enormous time :-).

After M13, I would prefer therefore, to have the "mass changes from the dtls connection id redesign" in branch https://github.com/eclipse/californium/tree/wip_add_dtls_cid (issue #824) merged first, because it blocks to many lines in scandium.

boaks commented 5 years ago

My "requirement" for a changed behavior is, that it stays possible to get (also) inner events. Adding the message as parameter to the callbacks seems to be also a good idea. One idea to make the outer/inner event selectable would be a method "select". Or marker interfaces e.g. "InnerMessageObserver".

sbernard31 commented 5 years ago

Could you precise what you are calling outer event ? and what you are calling inner event ?

boaks commented 5 years ago

"outer event" is the event related to the outer Request. "inner event" is the event related to a internally created request for blockwise. e.g. for inner events "ready to sent" is called on every block request, for the outer only at the first. If the first or last inner event is reported as outer (changing the argument, if a message is added as argument), depends on the event. I require the inner stuff for optimizations of a blockwise transfer, but, if you don't want such an optimization, you may only register for outer events.

sbernard31 commented 5 years ago

I try to reformulate to be sure I well understand :

When transparent blockwise is used, block requests are generated automatically by blockwise layer from a "none blockwise" request. What you are calling outer request is the "none blockwise/parent" request and inner requests are "blockwise/children" requests, right ?

Currently, Californium user does not really see inner requests, right? (except maybe with advanced monitoring feature like MessageInterceptor)

So questions are :

About solution, without thinking about it so much I would prefer the marker interface solution. If you add a classic MessageObserver you get only "high level" event about outer/parent request. If you add DetailedMessageObserver you get all detailed events (with inner event request). We could imagine to add parameters like outer/parent request and inner/child request (to know which is concerned by the event) This is just ideas, not strong opinion.

boaks commented 5 years ago

Currently, Californium user does not really see inner requests, right?

The inner Requests itself is not seen, but the events could be seen by the user. And that's not by mistake, I need it to monitor the blockwise progress and a small couple others also.

About solution, without thinking about it so much I would prefer the marker interface solution.

That would also be my preference. onResponse for inner request will then be called with the blockwise responses.

sbernard31 commented 4 years ago

(maybe this https://github.com/eclipse/californium/issues/1223 should be taking into account in one day we address this issue)