Hi All,
I
agree with Simon on solution 1. It doesn’t solve the
problem.
On
a closer look I see two variables participating in an
invariant which should always hold irrespective of which
thread sees it.
Invariant:
A message queue always exists as long as the client
exists.
The
problem mentioned below is a cause of this invariant being
violated.
So
the only solution to this is to make sure that operations
“checking client exists” and “adding to message store”
atomic. This atomic operation and unregister() can be
guarded with a lock. This is similar to what the solution 2
suggests. I think this will be better rather than making the
entire queue mode single threaded (thread-confinement as
suggested in solution 3) which is definitely slower than
solution 2.
Also,
there is one more minor issue. Say the above mentioned
atomic operation is successful and the next request is sent
to the processing Executor (https://github.com/eclipse/leshan/blob/master/leshan-server-core/src/main/java/org/eclipse/leshan/server/queue/impl/QueuedRequestSender.java#L130),
a check is in place in RequestSendingTask.java
to make sure that the client is still valid. If the client
is not valid any more, the message is unfortunately left in
the queue itself. It has to be cleared.
I
can prepare a PR providing the above mentioned solution
provided no other arguments against it comes.
Regards,
Bala
Hi,
My 2 cents below :
1) I'm not sure about the first proposition, we will create
another concurrency issue (between the "check" and the
"remove"), with less consequence but we do not really control
what we do.
Between 2) and 3) it's hard to say as we can not really know
the impact on performance. If you choose "thread confinement"
I suppose we can do that by endpoint. (The same thread must
always handle the same endpoint)
I read the code again and I wonder if this is a good thing
to just ignore request sending when device is not registered ?
The application layer should know that the request will not be
queued/sent ?
Simon
(I will not be available the next 2 weeks)
Le 29/07/2016 à 13:08, Maier Daniel
(INST/ECS4) a écrit :
Hi all,
we discovered a
concurrency issue in the current queue mode
implementation: In QueuedRequestSender send() method there
gets checked if the client is still
registered in client
registry (https://github.com/eclipse/leshan/blob/master/leshan-server-core/src/main/java/org/eclipse/leshan/server/queue/impl/QueuedRequestSender.java#L124).
If the client is not
registered we do not store and send this request to
prevent having requests in the queue for no longer
existing clients.
Now if we assume two
threads t1 (user thread that calls send()) and t2
(unregister thread that calls unregister()):
- t1 passes the check
mentioned above, but request was not added to messageStore
yet
- t2 removes all
request from queue because client was unregistered
- t1 adds the request
to message store
Now we have a request
in the store without a corresponding registered client,
i.e. this request may stay there forever (especially if we
introduce persistence).
We see different
solutions for this issue. Because I am unsure which is the
best, I am interested in your opinion. The solutions we
see are the following:
- Just do the above
mentioned check after adding the request to the store. If
the client is not registered anymore, remove the request
again and do not send it.
- Introduce a common
lock in send() and unregister() method to ensure that no
new requests get sent while unregister. We could use
read/write locks there.
- As we had multiple
concurrency issues within queue mode already, introduce a
thread model called "thread confinement", i.e. we execute
all queue mode operations within
one single thread.
This may slow down execution, but makes maintenance and
future extensions of code much easier.
Are there any opinions
out there for which solution we should go? Or does anyone
have further ideas for a fix? We will provide a PR once we
agreed for a solution.
Best Regards,
Daniel
_______________________________________________
leshan-dev mailing list
leshan-dev@xxxxxxxxxxx
To change your delivery options, retrieve your password, or unsubscribe from this list, visit
https://dev.eclipse.org/mailman/listinfo/leshan-dev
