Skip to content
Permalink
Browse files
Teach CoreIPC how to handle messages that are larger than the pipe's …
…buffer

::GetOverlappedResult and ::ReadFile can fail with ERROR_MORE_DATA
when there is more data available on the pipe than was requested in
the read operation. In those cases, the appropriate response is to
perform another read operation to read the extra data. We now do this.

Also, MSDN says that, because we are doing asynchronous read
operations, we should not pass a pointer to ::ReadFile to find out how
many bytes were read. Instead we should always call
::GetOverlappedResult to find this out. I've changed
Connection::readEventHandler to have a single loop that calls
::GetOverlappedResult and ::ReadFile in alternation, rather than
sometimes calling ::ReadFile multiple times in a row, to satisfy this
requirement.

In order to simplify the logic in this function, I've made us request
only a single byte from the pipe when there are no messages already in
the pipe. (Previously we were requesting 4096 bytes in this case.)
This allows us not to have to consider the case where the received
message is smaller than our read buffer. If we decide that this has a
negative impact on performance, we can of course change it. I've
mitigated this somewhat by using ::PeekNamedMessage to find out the
size of the next message in the pipe (if any), so that we can read it
all in one read operation.

Fixes <http://webkit.org/b/42710> <rdar://problem/8197571> Assertion
in Connection::readEventHandler when launching WebKitTestRunner

Reviewed by Anders Carlsson.

* Platform/CoreIPC/win/ConnectionWin.cpp:
(CoreIPC::Connection::readEventHandler): Put the call to
::GetOverlappedResult in the same loop as ::ReadFile so that we will
call them alternately. If ::GetOverlappedResult fails with
ERROR_MORE_DATA, use ::PeekNamedPipe to determine the size of the rest
of the message, then read it from the pipe. After dispatching the
message, use ::PeekNamedPipe to find out the size of the next message
in the pipe so we can read it all in one operation. If there's no
message in the pipe, we'll request just a single byte of the next
message that becomes available, and Windows will tell us when the rest
of the message is ready. If ::ReadFile fails with ERROR_MORE_DATA it
means there is data available now even though we didn't think there
was any. We go back to the top of the loop in this case and call
::GetOverlappedResult again to retrieve the available data.

Canonical link: https://commits.webkit.org/54689@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@63852 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
aroben committed Jul 21, 2010
1 parent c0f674c commit afd77970feb3deed6aabcf18a98424ab22bad0e7
Showing with 122 additions and 19 deletions.
  1. +49 −0 WebKit2/ChangeLog
  2. +73 −19 WebKit2/Platform/CoreIPC/win/ConnectionWin.cpp
@@ -1,3 +1,52 @@
2010-07-21 Adam Roben <aroben@apple.com>

Teach CoreIPC how to handle messages that are larger than the pipe's
buffer

::GetOverlappedResult and ::ReadFile can fail with ERROR_MORE_DATA
when there is more data available on the pipe than was requested in
the read operation. In those cases, the appropriate response is to
perform another read operation to read the extra data. We now do this.

Also, MSDN says that, because we are doing asynchronous read
operations, we should not pass a pointer to ::ReadFile to find out how
many bytes were read. Instead we should always call
::GetOverlappedResult to find this out. I've changed
Connection::readEventHandler to have a single loop that calls
::GetOverlappedResult and ::ReadFile in alternation, rather than
sometimes calling ::ReadFile multiple times in a row, to satisfy this
requirement.

In order to simplify the logic in this function, I've made us request
only a single byte from the pipe when there are no messages already in
the pipe. (Previously we were requesting 4096 bytes in this case.)
This allows us not to have to consider the case where the received
message is smaller than our read buffer. If we decide that this has a
negative impact on performance, we can of course change it. I've
mitigated this somewhat by using ::PeekNamedMessage to find out the
size of the next message in the pipe (if any), so that we can read it
all in one read operation.

Fixes <http://webkit.org/b/42710> <rdar://problem/8197571> Assertion
in Connection::readEventHandler when launching WebKitTestRunner

Reviewed by Anders Carlsson.

* Platform/CoreIPC/win/ConnectionWin.cpp:
(CoreIPC::Connection::readEventHandler): Put the call to
::GetOverlappedResult in the same loop as ::ReadFile so that we will
call them alternately. If ::GetOverlappedResult fails with
ERROR_MORE_DATA, use ::PeekNamedPipe to determine the size of the rest
of the message, then read it from the pipe. After dispatching the
message, use ::PeekNamedPipe to find out the size of the next message
in the pipe so we can read it all in one operation. If there's no
message in the pipe, we'll request just a single byte of the next
message that becomes available, and Windows will tell us when the rest
of the message is ready. If ::ReadFile fails with ERROR_MORE_DATA it
means there is data available now even though we didn't think there
was any. We go back to the top of the loop in this case and call
::GetOverlappedResult again to retrieve the available data.

2010-07-21 Sam Weinig <sam@webkit.org>

Reviewed by Anders Carlsson.
@@ -108,16 +108,40 @@ void Connection::readEventHandler()
sendOutgoingMessages();
}

DWORD numberOfBytesRead = 0;
if (wasConnected) {
while (true) {
// Check if we got some data.
DWORD numberOfBytesRead = 0;
if (!::GetOverlappedResult(m_connectionPipe, &m_readState, &numberOfBytesRead, FALSE)) {
DWORD error = ::GetLastError();

switch (error) {
case ERROR_BROKEN_PIPE:
connectionDidClose();
return;
case ERROR_MORE_DATA: {
// Read the rest of the message out of the pipe.

DWORD bytesToRead = 0;
if (!::PeekNamedPipe(m_connectionPipe, 0, 0, 0, 0, &bytesToRead)) {
DWORD error = ::GetLastError();
ASSERT_NOT_REACHED();
return;
}

// ::GetOverlappedResult told us there's more data. ::PeekNamedPipe shouldn't
// contradict it!
ASSERT(bytesToRead);
if (!bytesToRead)
break;

m_readBuffer.grow(m_readBuffer.size() + bytesToRead);
if (!::ReadFile(m_connectionPipe, m_readBuffer.data() + numberOfBytesRead, bytesToRead, 0, &m_readState)) {
DWORD error = ::GetLastError();
ASSERT_NOT_REACHED();
return;
}
continue;
}

// FIXME: We should figure out why we're getting this error.
case ERROR_IO_INCOMPLETE:
@@ -126,39 +150,69 @@ void Connection::readEventHandler()
ASSERT_NOT_REACHED();
}
}
}

while (true) {
if (numberOfBytesRead) {
if (!m_readBuffer.isEmpty()) {
// We have a message, let's dispatch it.

// The messageID is encoded at the end of the buffer.
size_t realBufferSize = numberOfBytesRead - sizeof(MessageID);
// Note that we assume here that the message is the same size as m_readBuffer. We can
// assume this because we always size m_readBuffer to exactly match the size of the message,
// either when receiving ERROR_MORE_DATA from ::GetOverlappedResult above or when
// ::PeekNamedPipe tells us the size below. We never set m_readBuffer to a size larger
// than the message.
size_t realBufferSize = m_readBuffer.size() - sizeof(MessageID);

unsigned messageID = *reinterpret_cast<unsigned*>(m_readBuffer.data() + realBufferSize);

processIncomingMessage(MessageID::fromInt(messageID), adoptPtr(new ArgumentDecoder(m_readBuffer.data(), realBufferSize)));
}

// FIXME: Do this somewhere else.
m_readBuffer.resize(inlineMessageMaxSize);
// Find out the size of the next message in the pipe (if there is one) so that we can read
// it all in one operation. (This is just an optimization to avoid an extra pass through the
// loop (if we chose a buffer size that was too small) or allocating extra memory (if we
// chose a buffer size that was too large).)
DWORD bytesToRead = 0;
if (!::PeekNamedPipe(m_connectionPipe, 0, 0, 0, 0, &bytesToRead)) {
DWORD error = ::GetLastError();
ASSERT_NOT_REACHED();
}
if (!bytesToRead) {
// There's no message waiting in the pipe. Schedule a read of the first byte of the
// next message. We'll find out the message's actual size when it arrives. (If we
// change this to read more than a single byte for performance reasons, we'll have to
// deal with m_readBuffer potentially being larger than the message we read after
// calling ::GetOverlappedResult above.)
bytesToRead = 1;
}

// Read from the pipe.
BOOL result = ::ReadFile(m_connectionPipe, m_readBuffer.data(), m_readBuffer.size(), &numberOfBytesRead, &m_readState);
m_readBuffer.resize(bytesToRead);

if (!result) {
DWORD error = ::GetLastError();
// Either read the next available message (which should occur synchronously), or start an
// asynchronous read of the next message that becomes available.
BOOL result = ::ReadFile(m_connectionPipe, m_readBuffer.data(), m_readBuffer.size(), 0, &m_readState);
if (result) {
// There was already a message waiting in the pipe, and we read it synchronously.
// Process it.
continue;
}

if (error == ERROR_IO_PENDING) {
// There's pending data - readEventHandler will be called again once the read is complete.
return;
}
DWORD error = ::GetLastError();

// FIXME: We need to handle other errors here.
ASSERT_NOT_REACHED();
if (error == ERROR_IO_PENDING) {
// There are no messages in the pipe currently. readEventHandler will be called again once there is a message.
return;
}

if (error == ERROR_MORE_DATA) {
// Either a message is available when we didn't think one was, or the message is larger
// than ::PeekNamedPipe told us. The former seems far more likely. Probably the message
// became available between our calls to ::PeekNamedPipe and ::ReadFile above. Go back
// to the top of the loop to use ::GetOverlappedResult to retrieve the available data.
continue;
}

ASSERT(numberOfBytesRead);
// FIXME: We need to handle other errors here.
ASSERT_NOT_REACHED();
}
}

0 comments on commit afd7797

Please sign in to comment.