Peer::Close() can get stuck waiting for the pending request semaphore #350
Assignees
Labels
kind/bug
This issue is a bug
Comments
yugabyte-ci
pushed a commit
that referenced
this issue
Jul 7, 2018
Summary: This a port from KUDU-699 apache/kudu@a32ea34 Test Plan: New unit test. Reviewers: sergei, mikhail Reviewed By: mikhail Subscribers: kannan, ybase, bharat Differential Revision: https://phabricator.dev.yugabyte.com/D5095
yugabyte-ci
pushed a commit
that referenced
this issue
Jul 9, 2018
Summary: This reverts commit 4f445a3. We saw a huge performance degradation with this commit. Reverting for now until we find a better solution. Test Plan: Verified that code builds correctly Reviewers: mikhail, sergei, kannan Reviewed By: kannan Subscribers: ybase, bharat Differential Revision: https://phabricator.dev.yugabyte.com/D5124
|
We found a performance regression with commit 4f445a3. Reverting it and reopening the issue. |
mbautin
pushed a commit
that referenced
this issue
Jul 13, 2018
Summary: This a port from KUDU-699 apache/kudu@a32ea34 Test Plan: New unit test. Reviewers: sergei, mikhail Reviewed By: mikhail Subscribers: kannan, ybase, bharat Differential Revision: https://phabricator.dev.yugabyte.com/D5095
yugabyte-ci
pushed a commit
that referenced
this issue
Jul 14, 2018
Summary: Currently we were submitting a call to the raft thread pool to call SendNextRequest. This is unnecessary if there is a pending request. This is a followup commit for github issue #350. Test Plan: Ran ``` /bin/yb-ctl destroy; sleep 2; ./bin/yb-ctl start; sleep 10; time java -jar java/yb-loadtester/target/yb-sample-apps.jar -workload CassandraKeyValue -num_threads_read 0 -num_threads_write 128 -nodes 127.0.0.1:9042,127.0.0.2:9042,127.0.0.3:9042 --num_writes 5000000 ``` 10 times, execution time went from 190s to 175s. Reviewers: pritam.damania, bogdan, mikhail Reviewed By: mikhail Subscribers: ybase, bharat Differential Revision: https://phabricator.dev.yugabyte.com/D5152
yugabyte-ci
pushed a commit
that referenced
this issue
Jul 16, 2018
Summary: Revert "Improve performance when sending peer's requests (#350)" This reverts commit 5a062dd54a568c29903f2262e413d5f66c0f7cb8. Revert "ENG-3422: #350 Remove semaphore from Peer class" This reverts commit ebd5508b07dd541d48c425977cf4f56621b44357. Test Plan: Verify that it builds Reviewers: sergei, bogdan, mikhail Reviewed By: mikhail Subscribers: bharat, ybase Differential Revision: https://phabricator.dev.yugabyte.com/D5159
yugabyte-ci
pushed a commit
that referenced
this issue
Jul 16, 2018
Summary: With this change we avoid blocking in Peer::Close() by creating a reference to itself in case it can't acquire the semaphore. This guarantees that the object stays alive until the pending request can be processed. Test Plan: Unit tests. Java workloads Reviewers: sergei, bogdan, mikhail Reviewed By: mikhail Subscribers: kannan, ybase Differential Revision: https://phabricator.dev.yugabyte.com/D5158
yugabyte-ci
pushed a commit
that referenced
this issue
Jul 16, 2018
Summary: Revert "Improve performance when sending peer's requests (#350)" This reverts commit 5a062dd54a568c29903f2262e413d5f66c0f7cb8. Revert "ENG-3422: #350 Remove semaphore from Peer class" This reverts commit ebd5508b07dd541d48c425977cf4f56621b44357. Test Plan: Verify that it builds Reviewers: sergei, bogdan, mikhail Reviewed By: mikhail Subscribers: bharat, ybase Differential Revision: https://phabricator.dev.yugabyte.com/D5159
yugabyte-ci
pushed a commit
that referenced
this issue
Aug 3, 2018
Summary: The Peer class uses a semaphore to enforce that there is only one outstanding request. Peer::Close() waits to acquire this semaphore before cleaning up its resources. By waiting on this semaphore, we guarantee that we won't deallocate resources while another thread is using them. But in some clusters we have found that sometimes we wait on this semaphore forever (most likely because we are waiting for a request response that never arrives or times out). Our first attempt was to port apache/kudu@a32ea34 but it caused a huge performance degradation (~30%) in write-only workloads, and our TSAN tests found that the fix is not correct because Peer::Close() could finish and the Peer object destroyed before proxy_->UpdateAsync() gets called since the lock has been released. With this change Peer::Close() marks the state of the peer as closed, and then waits for the semaphore only when the Peer is not waiting for a request response. This will not block indefinitely because as soon as the thread processing the requests sees that the Peer has been closed, it will refrain from sending more requests. If Peer::Close() gets called while we are waiting for a response (I have added waiting_for_response_ to track this), then we avoid waiting on the semaphore, and we free most of the resources as they are not being used by another thread. Before exiting Peer::Close() we will create an extra reference to the Peer object by calling shared_from_this() and storing the value in shared_from_this_. This will guarantee that the object stays alive until we process the response we are waiting for. As soon as Peer::ProcessResponse() or Peer::ProcessRemoteBootstrapResponse() see that the Peer has been closed, they erase the extra reference stored in shared_from_this_ and return. For correctness, we hold the spinlock while calling proxy_->UpdateAsync(), but this means that now we have to make this call on a different thread since UpdateAsync is not really an async call. In some cases it can call Peer::ProcessResponse in the same callstack which would cause a deadlock since ProcessResponse acquires the same lock that is being held while calling proxy_->UpdateAsync(). Test Plan: Unit tests. Java workloads Reviewers: sergei, bogdan, mikhail Reviewed By: mikhail Subscribers: kannan, ybase Differential Revision: https://phabricator.dev.yugabyte.com/D5158
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The following issue was observed on a client request.
GetTableLocations(system_redis.redis, hash_code: NaN, 5) failed: timed out after deadline expired: GetTableLocations(system_redis.redis, hash_code: NaN, 5) passed its deadline 7313896.654s (now: 7313897.511s): Remote error (yb/rpc/outbound_call.cc:376): Service unavailable (yb/rpc/service_pool.cc:170): GetTableLocations request on yb.master.MasterService from ip_address:53662 dropped due to backpressure. The service queue is full, it has 1000 items.
The master in question got into a state where it was not able to process any MasterService or Consensus requests.
List of master threads: https://gist.githubusercontent.com/mbautin/9b018f4ea37ee31aa80fed1b7f9f947f/raw
Looking at the thread dump of one of the threads waiting for the Raft state lock, we can see the thread holding that lock:
The second thread is handling
Peer::Closeand is waiting for the semaphore:This semaphore should be released when we receive a response from the peer, even if that response is a network error or a timeout. The time by which the response should be received has long passed. So we either failed to release the semaphore, or we never handled the response because it got stuck in one of the queues.
One possibility is that the thread running
Peer::Close()is doing so in a Raft thread pool token, and that token is also needed to handle the response inPeer::ProcessResponse(). That's when the semaphore should be released but we never get to that point.The text was updated successfully, but these errors were encountered: