Skip to content

GitLab

T
tangled
Commit 4ae17141 authored by Matt Menke's avatar Matt Menke Committed by Commit Bot
Browse files
Options

Reland "Remove SpawnedTestServer timeouts." 

This reverts commit f6ec475d.

Reason for revert: We're seeing these failures on the tree again.

Original change's description:
> Revert "Remove SpawnedTestServer timeouts."
> 
> This reverts commit 8e4bd238.
> 
> Reason for revert: want to verify that workarounds to https://crbug.com/869227 inside vpython itself are actually useful
> plus to read more logs.
> 
> The original can re-landed any time if new occurrences of https://crbug.com/869227 manifest themselves.
> 
> Original change's description:
> > Remove SpawnedTestServer timeouts.
> > 
> > These timeouts are ancient. The test infrastructure has its own
> > timeouts, which should make these obsolete. Also, this may help with
> > https://crbug.com/869227, where starting the Python test server
> > times out.
> > 
> > Bug: 869227
> > Change-Id: I9ea4f3174bcaa86daf09b965c82921432d8d556a
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1869717
> > Commit-Queue: Matt Menke <mmenke@chromium.org>
> > Reviewed-by: David Benjamin <davidben@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#707894}
> 
> TBR=davidben@chromium.org,mmenke@chromium.org
> 
> # Not skipping CQ checks because original CL landed > 1 day ago.
> 
> Bug: 869227
> Change-Id: Ic11c2e9a78568738a7712a1141876840f8a63740
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1893789
> Auto-Submit: Andrii Shyshkalov <tandrii@google.com>
> Reviewed-by: Robbie Iannucci <iannucci@chromium.org>
> Commit-Queue: Robbie Iannucci <iannucci@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#711706}

TBR=davidben@chromium.org,iannucci@chromium.org,tandrii@google.com,mmenke@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 869227
Change-Id: I31794b424e1d3aec994fea74f63645f85cb1b80a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1897976Reviewed-by: default avatarMatt Menke <mmenke@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#712233}
parent 06fea5ac
Hide whitespace changes
Inline Side-by-side
Showing with 6 additions and 75 deletions
net/test/spawned_test_server/local_test_server_posix.cc
View file @ 4ae17141
......@@ -19,7 +19,6 @@
#include "base/process/process_iterator.h"
#include "base/strings/string_number_conversions.h"
#include "base/strings/string_util.h"
#include "base/test/test_timeouts.h"
#include "net/test/python_utils.h"
namespace {
......@@ -59,13 +58,9 @@ class OrphanedTestServerFilter : public base::ProcessFilter {
// Given a file descriptor, reads into |buffer| until |bytes_max|
// bytes has been read or an error has been encountered. Returns true
// if the read was successful. |remaining_time| is used as a timeout.
bool ReadData(int fd,
ssize_t bytes_max,
uint8_t* buffer,
base::TimeDelta* remaining_time) {
// if the read was successful.
bool ReadData(int fd, ssize_t bytes_max, uint8_t* buffer) {
ssize_t bytes_read = 0;
base::TimeTicks previous_time = base::TimeTicks::Now();
while (bytes_read < bytes_max) {
struct pollfd poll_fds[1];
......@@ -73,8 +68,8 @@ bool ReadData(int fd,
poll_fds[0].events = POLLIN | POLLPRI;
poll_fds[0].revents = 0;
int rv = HANDLE_EINTR(poll(poll_fds, 1,
remaining_time->InMilliseconds()));
// Each test itself has its own timeout, so no need to use one here.
int rv = HANDLE_EINTR(poll(poll_fds, 1, -1));
if (rv == 0) {
LOG(ERROR) << "poll() timed out; bytes_read=" << bytes_read;
return false;
......@@ -84,12 +79,6 @@ bool ReadData(int fd,
return false;
}
base::TimeTicks current_time = base::TimeTicks::Now();
base::TimeDelta elapsed_time_cycle = current_time - previous_time;
DCHECK_GE(elapsed_time_cycle.InMilliseconds(), 0);
*remaining_time -= elapsed_time_cycle;
previous_time = current_time;
ssize_t num_bytes = HANDLE_EINTR(read(fd, buffer + bytes_read,
bytes_max - bytes_read));
if (num_bytes <= 0)
......@@ -163,18 +152,15 @@ bool LocalTestServer::LaunchPython(
bool LocalTestServer::WaitToStart() {
base::ScopedFD our_fd(child_fd_.release());
base::TimeDelta remaining_time = TestTimeouts::action_timeout();
uint32_t server_data_len = 0;
if (!ReadData(our_fd.get(), sizeof(server_data_len),
reinterpret_cast<uint8_t*>(&server_data_len),
&remaining_time)) {
reinterpret_cast<uint8_t*>(&server_data_len))) {
LOG(ERROR) << "Could not read server_data_len";
return false;
}
std::string server_data(server_data_len, '\0');
if (!ReadData(our_fd.get(), server_data_len,
reinterpret_cast<uint8_t*>(&server_data[0]), &remaining_time)) {
reinterpret_cast<uint8_t*>(&server_data[0]))) {
LOG(ERROR) << "Could not read server_data (" << server_data_len
<< " bytes)";
return false;
......
net/test/spawned_test_server/local_test_server_win.cc
View file @ 4ae17141
......@@ -13,32 +13,14 @@
#include "base/files/file_path.h"
#include "base/path_service.h"
#include "base/process/launch.h"
#include "base/single_thread_task_runner.h"
#include "base/strings/string_number_conversions.h"
#include "base/strings/string_util.h"
#include "base/strings/utf_string_conversions.h"
#include "base/test/test_timeouts.h"
#include "base/threading/thread.h"
#include "base/threading/thread_restrictions.h"
#include "base/win/scoped_handle.h"
#include "net/test/python_utils.h"
namespace {
// Writes |size| bytes to |handle| and sets |*unblocked| to true.
// Used as a crude timeout mechanism by ReadData().
void UnblockPipe(HANDLE handle, DWORD size, bool* unblocked) {
std::string unblock_data(size, '\0');
// Unblock the ReadFile in LocalTestServer::WaitToStart by writing to the
// pipe. Make sure the call succeeded, otherwise we are very likely to hang.
DWORD bytes_written = 0;
LOG(WARNING) << "Timeout reached; unblocking pipe by writing "
<< size << " bytes";
CHECK(WriteFile(handle, unblock_data.data(), size, &bytes_written, nullptr));
CHECK_EQ(size, bytes_written);
*unblocked = true;
}
// Given a file handle, reads into |buffer| until |bytes_max| bytes
// has been read or an error has been encountered. Returns
// true if the read was successful.
......@@ -46,16 +28,6 @@ bool ReadData(HANDLE read_fd,
HANDLE write_fd,
DWORD bytes_max,
uint8_t* buffer) {
base::Thread thread("test_server_watcher");
if (!thread.Start())
return false;
// Prepare a timeout in case the server fails to start.
bool unblocked = false;
thread.task_runner()->PostDelayedTask(
FROM_HERE, base::BindOnce(UnblockPipe, write_fd, bytes_max, &unblocked),
TestTimeouts::action_max_timeout());
DWORD bytes_read = 0;
while (bytes_read < bytes_max) {
DWORD num_bytes;
......@@ -71,15 +43,6 @@ bool ReadData(HANDLE read_fd,
bytes_read += num_bytes;
}
base::ScopedAllowBaseSyncPrimitivesForTesting allow_thread_join;
thread.Stop();
// If the timeout kicked in, abort.
if (unblocked) {
LOG(ERROR) << "Timeout exceeded for ReadData";
return false;
}
return true;
}
......
net/test/spawned_test_server/remote_test_server_spawner_request.cc
View file @ 4ae17141
......@@ -12,10 +12,7 @@
#include "base/macros.h"
#include "base/single_thread_task_runner.h"
#include "base/synchronization/waitable_event.h"
#include "base/test/test_timeouts.h"
#include "base/threading/thread_task_runner_handle.h"
#include "base/time/time.h"
#include "base/timer/timer.h"
#include "build/build_config.h"
#include "net/base/elements_upload_data_stream.h"
#include "net/base/io_buffer.h"
......@@ -50,7 +47,6 @@ class RemoteTestServerSpawnerRequest::Core : public URLRequest::Delegate {
void ReadResponse();
void OnCommandCompleted(int net_error);
void OnTimeout();
// Request results.
int result_code_ = 0;
......@@ -64,8 +60,6 @@ class RemoteTestServerSpawnerRequest::Core : public URLRequest::Delegate {
scoped_refptr<IOBuffer> read_buffer_;
std::unique_ptr<base::OneShotTimer> timeout_timer_;
THREAD_CHECKER(thread_checker_);
DISALLOW_COPY_AND_ASSIGN(Core);
......@@ -102,10 +96,6 @@ void RemoteTestServerSpawnerRequest::Core::SendRequest(
/*override=*/true);
}
timeout_timer_ = std::make_unique<base::OneShotTimer>();
timeout_timer_->Start(FROM_HERE, TestTimeouts::action_max_timeout(),
base::Bind(&Core::OnTimeout, base::Unretained(this)));
request_->Start();
}
......@@ -124,13 +114,6 @@ bool RemoteTestServerSpawnerRequest::Core::WaitForCompletion(
return result_code_ == OK;
}
void RemoteTestServerSpawnerRequest::Core::OnTimeout() {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
int result = request_->CancelWithError(ERR_TIMED_OUT);
OnCommandCompleted(result);
}
void RemoteTestServerSpawnerRequest::Core::OnCommandCompleted(int net_error) {
DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
DCHECK_NE(ERR_IO_PENDING, net_error);
......@@ -152,7 +135,6 @@ void RemoteTestServerSpawnerRequest::Core::OnCommandCompleted(int net_error) {
request_.reset();
context_.reset();
timeout_timer_.reset();
event_.Signal();
}
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment