Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setReadyToSent call several times with transparent block mode. #818

Open
sbernard31 opened this issue Dec 3, 2018 · 10 comments
Open

setReadyToSent call several times with transparent block mode. #818

sbernard31 opened this issue Dec 3, 2018 · 10 comments
Labels

Comments

@sbernard31
Copy link
Contributor

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 : eclipse-leshan/leshan#623)

@boaks
Copy link
Contributor

boaks commented Dec 3, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor

boaks commented Jan 14, 2019

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

@boaks
Copy link
Contributor

boaks commented Jan 17, 2019

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
Copy link
Contributor

boaks commented Jan 21, 2019

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
Copy link
Contributor Author

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

@boaks
Copy link
Contributor

boaks commented Jan 22, 2019

"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
Copy link
Contributor Author

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 :

  • how could we correctly handle this inner/outer request concepts at MessageObserver level ?
  • how inner request event impact outer request and vice versa? (e.g. cancel outer request cancel inner request, failed to send inner request make fail the outer request ...)
    (a bonus implementation concern is about exchange complete stuff)

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
Copy link
Contributor

boaks commented Jan 22, 2019

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
Copy link
Contributor Author

(maybe this #1223 should be taking into account in one day we address this issue)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants