diff options
author | Sebastien Hertz <shertz@google.com> | 2015-03-24 19:03:40 +0100 |
---|---|---|
committer | Sebastien Hertz <shertz@google.com> | 2015-04-01 12:22:52 +0200 |
commit | 4e5b20863898006ec6c9d120cda167d38dda6e60 (patch) | |
tree | 8cb7e98c87a4e48e6237fd2c172ae39f5b7f5a76 /runtime/jdwp | |
parent | 04914da1385564fca1990863d9a2690af10e1946 (diff) | |
download | art-4e5b20863898006ec6c9d120cda167d38dda6e60.tar.gz art-4e5b20863898006ec6c9d120cda167d38dda6e60.tar.bz2 art-4e5b20863898006ec6c9d120cda167d38dda6e60.zip |
Fix JDWP race at runtime shutdown
When the runtime shuts down, it closes the JDWP connection with the
debugger. However, if a JDWP command is still being processed by the
JDWP handler thread when we close the connection, we won't be able to
send its reply.
Bug: 19628620
Change-Id: I20301325a347d66f3b9ef95ebe8f156abafb1f76
Diffstat (limited to 'runtime/jdwp')
-rw-r--r-- | runtime/jdwp/jdwp.h | 8 | ||||
-rw-r--r-- | runtime/jdwp/jdwp_handler.cc | 2 | ||||
-rw-r--r-- | runtime/jdwp/jdwp_main.cc | 36 |
3 files changed, 40 insertions, 6 deletions
diff --git a/runtime/jdwp/jdwp.h b/runtime/jdwp/jdwp.h index e16221c69a..31c9a0bb4e 100644 --- a/runtime/jdwp/jdwp.h +++ b/runtime/jdwp/jdwp.h @@ -403,6 +403,14 @@ struct JdwpState { // Used for VirtualMachine.Exit command handling. bool should_exit_; int exit_status_; + + // Used to synchronize runtime shutdown with JDWP command handler thread. + // When the runtime shuts down, it needs to stop JDWP command handler thread by closing the + // JDWP connection. However, if the JDWP thread is processing a command, it needs to wait + // for the command to finish so we can send its reply before closing the connection. + Mutex shutdown_lock_ ACQUIRED_AFTER(event_list_lock_); + ConditionVariable shutdown_cond_ GUARDED_BY(shutdown_lock_); + bool processing_request_ GUARDED_BY(shutdown_lock_); }; std::string DescribeField(const FieldId& field_id) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_); diff --git a/runtime/jdwp/jdwp_handler.cc b/runtime/jdwp/jdwp_handler.cc index add1394f2d..c7113912d3 100644 --- a/runtime/jdwp/jdwp_handler.cc +++ b/runtime/jdwp/jdwp_handler.cc @@ -271,7 +271,7 @@ static JdwpError VM_IDSizes(JdwpState*, Request*, ExpandBuf* pReply) static JdwpError VM_Dispose(JdwpState*, Request*, ExpandBuf*) SHARED_LOCKS_REQUIRED(Locks::mutator_lock_) { - Dbg::Disposed(); + Dbg::Dispose(); return ERR_NONE; } diff --git a/runtime/jdwp/jdwp_main.cc b/runtime/jdwp/jdwp_main.cc index e2b88a5e79..5b30f0cd8c 100644 --- a/runtime/jdwp/jdwp_main.cc +++ b/runtime/jdwp/jdwp_main.cc @@ -126,6 +126,7 @@ void JdwpNetStateBase::Close() { */ ssize_t JdwpNetStateBase::WritePacket(ExpandBuf* pReply, size_t length) { MutexLock mu(Thread::Current(), socket_lock_); + DCHECK(IsConnected()) << "Connection with debugger is closed"; DCHECK_LE(length, expandBufGetLength(pReply)); return TEMP_FAILURE_RETRY(write(clientSock, expandBufGetBuffer(pReply), length)); } @@ -140,6 +141,7 @@ ssize_t JdwpNetStateBase::WriteBufferedPacket(const std::vector<iovec>& iov) { ssize_t JdwpNetStateBase::WriteBufferedPacketLocked(const std::vector<iovec>& iov) { socket_lock_.AssertHeld(Thread::Current()); + DCHECK(IsConnected()) << "Connection with debugger is closed"; return TEMP_FAILURE_RETRY(writev(clientSock, &iov[0], iov.size())); } @@ -225,7 +227,10 @@ JdwpState::JdwpState(const JdwpOptions* options) jdwp_token_owner_thread_id_(0), ddm_is_active_(false), should_exit_(false), - exit_status_(0) { + exit_status_(0), + shutdown_lock_("JDWP shutdown lock", kJdwpShutdownLock), + shutdown_cond_("JDWP shutdown condition variable", shutdown_lock_), + processing_request_(false) { } /* @@ -338,10 +343,20 @@ void JdwpState::ResetState() { JdwpState::~JdwpState() { if (netState != nullptr) { /* - * Close down the network to inspire the thread to halt. + * Close down the network to inspire the thread to halt. If a request is being processed, + * we need to wait for it to finish first. */ - VLOG(jdwp) << "JDWP shutting down net..."; - netState->Shutdown(); + { + Thread* self = Thread::Current(); + MutexLock mu(self, shutdown_lock_); + while (processing_request_) { + VLOG(jdwp) << "JDWP command in progress: wait for it to finish ..."; + shutdown_cond_.Wait(self); + } + + VLOG(jdwp) << "JDWP shutting down net..."; + netState->Shutdown(); + } if (debug_thread_started_) { run = false; @@ -369,7 +384,13 @@ bool JdwpState::IsActive() { // Returns "false" if we encounter a connection-fatal error. bool JdwpState::HandlePacket() { - JdwpNetStateBase* netStateBase = reinterpret_cast<JdwpNetStateBase*>(netState); + Thread* const self = Thread::Current(); + { + MutexLock mu(self, shutdown_lock_); + processing_request_ = true; + } + JdwpNetStateBase* netStateBase = netState; + CHECK(netStateBase != nullptr) << "Connection has been closed"; JDWP::Request request(netStateBase->input_buffer_, netStateBase->input_count_); ExpandBuf* pReply = expandBufAlloc(); @@ -388,6 +409,11 @@ bool JdwpState::HandlePacket() { } expandBufFree(pReply); netStateBase->ConsumeBytes(request.GetLength()); + { + MutexLock mu(self, shutdown_lock_); + processing_request_ = false; + shutdown_cond_.Broadcast(self); + } return true; } |