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