Skip to content
This repository has been archived by the owner on Jan 16, 2024. It is now read-only.

MessageRouter::send() does not take the m_connectionMutex #5

Closed
lrvega opened this issue Apr 13, 2017 · 3 comments
Closed

MessageRouter::send() does not take the m_connectionMutex #5

lrvega opened this issue Apr 13, 2017 · 3 comments
Assignees
Milestone

Comments

@lrvega
Copy link

lrvega commented Apr 13, 2017

Hello,

I was wondering if MessageRouter::send() should actually take the m_connectionMutex lock? When inspecting other methods in the class, when m_activeTransport is read and/or queried, m_connectionMutex is locked (recursively, but it is locked).

Thanks,
Luis

@scotthea-amazon
Copy link
Contributor

Hello lrvega,
Yes, that is correct. Thank you very much for pointing out this bug! We have a fix in progress that holds the lock just long enough to copy the shared pointer. It is along the following lines:

 void MessageRouter::send(std::shared_ptr<MessageRequest> request) {
     auto task = [this, request]() {
-        if (m_activeTransport && m_activeTransport->isConnected()) {
-            m_activeTransport->send(request);
+        std::unique_lock<std::recursive_mutex> lock{m_connectionMutex};
+        auto transport = m_activeTransport;
+        lock.unlock();
+        if (transport) {
+            transport->send(request);
         }
     };
     m_sendExecutor->submit(task);
 }

Thank you again!
-SWH

@JamieMeyers JamieMeyers added the bug label Jun 2, 2017
@yugoren yugoren added the ACL label Jun 2, 2017
@JamieMeyers
Copy link
Contributor

Hi @lrvega,

Sorry for the delay. This will go out in the next release.

Thanks,
Jamie

@scotthea-amazon scotthea-amazon self-assigned this Jun 8, 2017
kjkh pushed a commit that referenced this issue Jun 9, 2017
Changes in this update
-Implemented Sensory wake word detector functionality
-Removed the need for a std::recursive_mutex in MessageRouter
-Added AIP unit test
-Added handleDirectiveImmediately functionality to SpeechSynthesizer
-Added memory profiles for:
AIP
SpeechSynthesizer
ContextManager
AVSUtils
AVSCommon
-Bug fix for MultipartParser.h compiler warning
-Suppression of sensitive log data even in debug builds. Use cmake parameter -DACSDK_EMIT_SENSITIVE_LOGS=ON to allow logging of sensitive information in DEBUG builds
-Fix crash in ACL when attempting to use more than 10 streams
-Updated MediaPlayer to use autoaudiosink instead of requiring pulseaudio
-Updated MediaPlayer build to suppport local builds of GStreamer
-Fixes for the following Github issues:
#5
#8
#9
#10
#17
#24
@scotthea-amazon
Copy link
Contributor

This fix has been pushed in version 0.4.1.

@JamieMeyers JamieMeyers added this to the 0.4.1 milestone Jun 13, 2017
@baddemiya baddemiya mentioned this issue Feb 17, 2018
2 tasks
@adeb adeb mentioned this issue Apr 8, 2018
6 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants