From 57a2a2beff565c9a2e2ed0937df178c38d114cb3 Mon Sep 17 00:00:00 2001 From: Monroe Linden Date: Fri, 23 Apr 2010 15:50:22 -0700 Subject: Add a way for plugins to send a message and block waiting for the response This requires some cooperation between the plugin and the host, and will only work for specific messages. The way it works is as follows: * the plugin sends a message containing the key "blocking_request" (with any value) * this will cause the "send message" function to block (continuing to pull incoming messages off the network socket and queue them) until it receives a message from the host containing the key "blocking_response" ** NOTE: if the plugin sends a blocking_request that isn't set up to cause the host to send back a blocking_response, it will block forever * the blocking_response message will be handed to the plugin's "receive message" function _immediately_ (before the "send message" function returns) ** this means that the plugin can extract response data for use by the the code that sent the message (but is still blocked inside the "send message" call) ** NOTE: this BREAKS the invariant stating that the plugin's "receive message" function will never be called recursively, and the plugin MUST be prepared to deal with this * after the plugin finishes processing the blocking_response message, the "send message" function that was called with the blocking_request message will return to the plugin * any queued messages will be delivered after the outer invocation of the plugin's "receive message" function returns (as normal) Inside the viewer, the code can tell when a plugin is in this blocked state by calling LLPluginProcessParent::isBlocked(). LLPluginClassMedia uses this to avoid sending mouse-move and size-change messages to blocked plugins. --- indra/llplugin/llpluginmessagepipe.cpp | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) (limited to 'indra/llplugin/llpluginmessagepipe.cpp') diff --git a/indra/llplugin/llpluginmessagepipe.cpp b/indra/llplugin/llpluginmessagepipe.cpp index 1d7ddc5592..e524c88cf8 100644 --- a/indra/llplugin/llpluginmessagepipe.cpp +++ b/indra/llplugin/llpluginmessagepipe.cpp @@ -299,26 +299,23 @@ bool LLPluginMessagePipe::pump(F64 timeout) void LLPluginMessagePipe::processInput(void) { // Look for input delimiter(s) in the input buffer. - int start = 0; int delim; - while((delim = mInput.find(MESSAGE_DELIMITER, start)) != std::string::npos) + while((delim = mInput.find(MESSAGE_DELIMITER)) != std::string::npos) { // Let the owner process this message if (mOwner) { - mOwner->receiveMessageRaw(mInput.substr(start, delim - start)); + // Pull the message out of the input buffer before calling receiveMessageRaw. + // It's now possible for this function to get called recursively (in the case where the plugin makes a blocking request) + // and this guarantees that the messages will get dequeued correctly. + std::string message(mInput, 0, delim); + mInput.erase(0, delim + 1); + mOwner->receiveMessageRaw(message); } else { LL_WARNS("Plugin") << "!mOwner" << LL_ENDL; } - - start = delim + 1; } - - // Remove delivered messages from the input buffer. - if(start != 0) - mInput = mInput.substr(start); - } -- cgit v1.2.3 From dacc5afbf0f1bde7454c1eadf56edb669d0741a9 Mon Sep 17 00:00:00 2001 From: Monroe Linden Date: Tue, 27 Apr 2010 17:25:01 -0700 Subject: Architectural changes to LLPlugin message processing. LLPluginProcessParent can now optionally use a separate thread for reading messages from plugin sockets. If this is enabled, it will spawn a single thread and use apr_pollset_poll to wake up the thread when incoming data arrives instead of polling all the descriptors round-robin every frame. This should be somewhat more efficient, and should also allow blocking requests from plugins to be serviced much more quickly (once we start using them). This is currently disabled by default, until it's had a bit more focused testing on multiple platforms. Hooked up the switch to use the message read thread to the PluginUseReadThread debug setting and an item in the Advanced menu in the viewer, and to a checkbox in the UI in llmediaplugintest. Updated some debug logging in the plugin system to have appropriate tags and not log dire-looking warnings during normal operation. LLPluginProcessParent now once again explicitly kills plugin processes (instead of just closing their sockets and waiting for them to exit). The problem we were attempting to solve by not doing the kill (letting the webkit plugin write its cookie file on exit) has been solved another way. LLPluginProcessParent::sendMessage() now attempts to write the outgoing message to the socket immediately instead of waiting for the next frame. This should reduce the latency of sending plugin messages. Added a separate fast timer for LLViewerMedia::updateMedia(). --- indra/llplugin/llpluginmessagepipe.cpp | 107 +++++++++++++++++++++++++++------ 1 file changed, 87 insertions(+), 20 deletions(-) (limited to 'indra/llplugin/llpluginmessagepipe.cpp') diff --git a/indra/llplugin/llpluginmessagepipe.cpp b/indra/llplugin/llpluginmessagepipe.cpp index e524c88cf8..a62534de95 100644 --- a/indra/llplugin/llpluginmessagepipe.cpp +++ b/indra/llplugin/llpluginmessagepipe.cpp @@ -96,11 +96,14 @@ void LLPluginMessagePipeOwner::killMessagePipe(void) } } -LLPluginMessagePipe::LLPluginMessagePipe(LLPluginMessagePipeOwner *owner, LLSocket::ptr_t socket) +LLPluginMessagePipe::LLPluginMessagePipe(LLPluginMessagePipeOwner *owner, LLSocket::ptr_t socket): + mInputMutex(gAPRPoolp), + mOutputMutex(gAPRPoolp), + mOwner(owner), + mSocket(socket) { - mOwner = owner; + mOwner->setMessagePipe(this); - mSocket = socket; } LLPluginMessagePipe::~LLPluginMessagePipe() @@ -114,8 +117,10 @@ LLPluginMessagePipe::~LLPluginMessagePipe() bool LLPluginMessagePipe::addMessage(const std::string &message) { // queue the message for later output + mOutputMutex.lock(); mOutput += message; mOutput += MESSAGE_DELIMITER; // message separator + mOutputMutex.unlock(); return true; } @@ -148,6 +153,18 @@ void LLPluginMessagePipe::setSocketTimeout(apr_interval_time_t timeout_usec) } bool LLPluginMessagePipe::pump(F64 timeout) +{ + bool result = pumpOutput(); + + if(result) + { + result = pumpInput(timeout); + } + + return result; +} + +bool LLPluginMessagePipe::pumpOutput() { bool result = true; @@ -156,6 +173,7 @@ bool LLPluginMessagePipe::pump(F64 timeout) apr_status_t status; apr_size_t size; + mOutputMutex.lock(); if(!mOutput.empty()) { // write any outgoing messages @@ -183,6 +201,17 @@ bool LLPluginMessagePipe::pump(F64 timeout) // remove the written part from the buffer and try again later. mOutput = mOutput.substr(size); } + else if(APR_STATUS_IS_EOF(status)) + { + // This is what we normally expect when a plugin exits. + llinfos << "Got EOF from plugin socket. " << llendl; + + if(mOwner) + { + mOwner->socketError(status); + } + result = false; + } else { // some other error @@ -196,6 +225,20 @@ bool LLPluginMessagePipe::pump(F64 timeout) result = false; } } + mOutputMutex.unlock(); + } + + return result; +} + +bool LLPluginMessagePipe::pumpInput(F64 timeout) +{ + bool result = true; + + if(mSocket) + { + apr_status_t status; + apr_size_t size; // FIXME: For some reason, the apr timeout stuff isn't working properly on windows. // Until such time as we figure out why, don't try to use the socket timeout -- just sleep here instead. @@ -216,8 +259,16 @@ bool LLPluginMessagePipe::pump(F64 timeout) char input_buf[1024]; apr_size_t request_size; - // Start out by reading one byte, so that any data received will wake us up. - request_size = 1; + if(timeout == 0.0f) + { + // If we have no timeout, start out with a full read. + request_size = sizeof(input_buf); + } + else + { + // Start out by reading one byte, so that any data received will wake us up. + request_size = 1; + } // and use the timeout so we'll sleep if no data is available. setSocketTimeout((apr_interval_time_t)(timeout * 1000000)); @@ -236,11 +287,15 @@ bool LLPluginMessagePipe::pump(F64 timeout) // LL_INFOS("Plugin") << "after apr_socket_recv, size = " << size << LL_ENDL; if(size > 0) + { + mInputMutex.lock(); mInput.append(input_buf, size); + mInputMutex.unlock(); + } if(status == APR_SUCCESS) { -// llinfos << "success, read " << size << llendl; + LL_DEBUGS("PluginSocket") << "success, read " << size << LL_ENDL; if(size != request_size) { @@ -250,16 +305,28 @@ bool LLPluginMessagePipe::pump(F64 timeout) } else if(APR_STATUS_IS_TIMEUP(status)) { -// llinfos << "TIMEUP, read " << size << llendl; + LL_DEBUGS("PluginSocket") << "TIMEUP, read " << size << LL_ENDL; // Timeout was hit. Since the initial read is 1 byte, this should never be a partial read. break; } else if(APR_STATUS_IS_EAGAIN(status)) { -// llinfos << "EAGAIN, read " << size << llendl; + LL_DEBUGS("PluginSocket") << "EAGAIN, read " << size << LL_ENDL; - // We've been doing partial reads, and we're done now. + // Non-blocking read returned immediately. + break; + } + else if(APR_STATUS_IS_EOF(status)) + { + // This is what we normally expect when a plugin exits. + LL_INFOS("PluginSocket") << "Got EOF from plugin socket. " << LL_ENDL; + + if(mOwner) + { + mOwner->socketError(status); + } + result = false; break; } else @@ -276,22 +343,18 @@ bool LLPluginMessagePipe::pump(F64 timeout) break; } - // Second and subsequent reads should not use the timeout - setSocketTimeout(0); - // and should try to fill the input buffer - request_size = sizeof(input_buf); + if(timeout != 0.0f) + { + // Second and subsequent reads should not use the timeout + setSocketTimeout(0); + // and should try to fill the input buffer + request_size = sizeof(input_buf); + } } processInput(); } } - - if(!result) - { - // If we got an error, we're done. - LL_INFOS("Plugin") << "Error from socket, cleaning up." << LL_ENDL; - delete this; - } return result; } @@ -300,6 +363,7 @@ void LLPluginMessagePipe::processInput(void) { // Look for input delimiter(s) in the input buffer. int delim; + mInputMutex.lock(); while((delim = mInput.find(MESSAGE_DELIMITER)) != std::string::npos) { // Let the owner process this message @@ -310,12 +374,15 @@ void LLPluginMessagePipe::processInput(void) // and this guarantees that the messages will get dequeued correctly. std::string message(mInput, 0, delim); mInput.erase(0, delim + 1); + mInputMutex.unlock(); mOwner->receiveMessageRaw(message); + mInputMutex.lock(); } else { LL_WARNS("Plugin") << "!mOwner" << LL_ENDL; } } + mInputMutex.unlock(); } -- cgit v1.2.3 From 77b13dc2df679c46f648a4f99c8e4d8836983a48 Mon Sep 17 00:00:00 2001 From: Monroe Linden Date: Thu, 29 Apr 2010 17:33:37 -0700 Subject: Incorporate suggestions from Richard's review of the LLPlugin changes. Use LLMutexLock (stack-based locker/unlocker) for the straightforward cases instead of explicit lock()/unlock(). There are still a couple of cases (one overlapping lock lifetime and two loops that unlock the mutex to call another function inside the loop) where I'm leaving explicit lock/unlock calls. Rename LLPluginProcessParent::sPollThread to sReadThread, for consistency. Made the LLPluginProcessParent destructor hold mIncomingQueueMutex while removing the instance from the global list -- this should prevent a possible race condition in LLPluginProcessParent::poll(). Removed a redundant check when calling LLPluginProcessParent::setUseReadThread(). --- indra/llplugin/llpluginmessagepipe.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) (limited to 'indra/llplugin/llpluginmessagepipe.cpp') diff --git a/indra/llplugin/llpluginmessagepipe.cpp b/indra/llplugin/llpluginmessagepipe.cpp index a62534de95..89f8b44569 100644 --- a/indra/llplugin/llpluginmessagepipe.cpp +++ b/indra/llplugin/llpluginmessagepipe.cpp @@ -117,10 +117,9 @@ LLPluginMessagePipe::~LLPluginMessagePipe() bool LLPluginMessagePipe::addMessage(const std::string &message) { // queue the message for later output - mOutputMutex.lock(); + LLMutexLock lock(&mOutputMutex); mOutput += message; mOutput += MESSAGE_DELIMITER; // message separator - mOutputMutex.unlock(); return true; } @@ -173,7 +172,7 @@ bool LLPluginMessagePipe::pumpOutput() apr_status_t status; apr_size_t size; - mOutputMutex.lock(); + LLMutexLock lock(&mOutputMutex); if(!mOutput.empty()) { // write any outgoing messages @@ -225,7 +224,6 @@ bool LLPluginMessagePipe::pumpOutput() result = false; } } - mOutputMutex.unlock(); } return result; @@ -288,9 +286,8 @@ bool LLPluginMessagePipe::pumpInput(F64 timeout) if(size > 0) { - mInputMutex.lock(); + LLMutexLock lock(&mInputMutex); mInput.append(input_buf, size); - mInputMutex.unlock(); } if(status == APR_SUCCESS) -- cgit v1.2.3