traeak commented on code in PR #13806:
URL: https://github.com/apache/trafficserver/pull/13806#discussion_r4294770624
##########
src/iocore/net/UnixNetVConnection.cc:
##########
@@ -1311,43 +1311,55 @@ UnixNetVConnection::is_default_inactivity_timeout()
/*
* Close down the current netVC. Save aside the socket and SSL information
- * and create new netVC in the current thread/netVC
+ * so a new netVC can be created from them in another thread.
*/
-UnixNetVConnection *
-UnixNetVConnection::migrateToCurrentThread(Continuation *cont, EThread *t)
+void
+UnixNetVConnection::detachForMigration(MigrationState &ms)
{
- NetHandler *client_nh = get_NetHandler(t);
- ink_assert(client_nh);
- if (this->nh == client_nh) {
- // We're already there!
- return this;
- }
-
- Connection hold_con;
- hold_con.move(this->con);
-
- void *arg = this->_prepareForMigration();
+ ms.con.move(this->con);
+ ms.arg = this->_prepareForMigration();
+ ms.options = this->options;
+ ms.context = this->get_context();
+ ms.inactivity_timeout_in = this->inactivity_timeout_in;
+ ms.processor = this->_getNetProcessor();
// Do_io_close will signal the VC to be freed on the original thread
// Since we moved the con context, the fd will not be closed
- // Go ahead and remove the fd from the original thread's epoll structure, so
it is not
+ // Go ahead and remove the fd from the original thread's poller, so it is not
// processed on two threads simultaneously
- this->ep.stop();
+ //
+ // This stop is not synchronized with the original thread. That thread may
have already seen
+ // this VC as open and enabled in NetHandler::process_ready_list, in which
case it goes on to
+ // net_read_io, fails the try lock on the VIO mutex held by the caller, and
reschedules the VC.
+ // The reschedule calls ep.refresh on the EventIO stopped here, so
EventIO::modify and
+ // EventIO::refresh do nothing on a stopped EventIO. A delay before the try
lock in net_read_io
+ // makes the original thread hit this reliably.
+ //
+ // Unlike an ordinary close, migration keeps the fd open, so kqueue
registrations must
+ // also be removed here before the old VC can be freed.
+ ink_release_assert(this->ep.stop_for_migration() == 0);
Review Comment:
Done in f068f1f95. The stop now runs before the socket and SSL state are
moved out of the old VC, so on failure the VC still owns them and an ordinary
do_io_close() recovers. detachForMigration() logs an Error(), ink_asserts in
debug builds and returns false; _acquire_session() then drops the session and
increments origin_shutdown_migration_failure instead of aborting.
##########
src/iocore/net/EventIO.cc:
##########
@@ -102,10 +140,11 @@ EventIO::modify(int e)
EV_SET(&ev[n++], fd, EVFILT_WRITE, EV_ADD | INK_EV_EDGE_TRIGGER, 0, 0,
this);
}
events = ee;
- if (n)
- return kevent(event_loop->kqueue_fd, &ev[0], n, nullptr, 0, nullptr);
- else
+ if (n) {
+ return kevent(loop->kqueue_fd, &ev[0], n, nullptr, 0, nullptr);
Review Comment:
Added a comment in f068f1f95 saying this level-triggered kqueue path is not
race safe against stop_for_migration() and would need the same undo as
refresh() before it is built. Since USE_EDGE_TRIGGER is hard defined for kqueue
the path is not compiled, so I did not add untestable handling.
##########
src/iocore/net/unit_tests/test_EventIOMigration.cc:
##########
@@ -0,0 +1,87 @@
+/** @file
+
+ Tests for kqueue registration teardown when a socket is migrated.
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+ */
+
+#include "../P_UnixPollDescriptor.h"
+#include "iocore/net/EventIO.h"
+#include "iocore/net/ReadWriteEventIO.h"
+
+#include <catch2/catch_test_macros.hpp>
+
+#include <cerrno>
+#include <unistd.h>
+
+#if TS_USE_KQUEUE
Review Comment:
Added tests/gold_tests/session_sharing/pool_lock.test.py in f068f1f95 with
ATSReplayTest replay files for the global and hybrid pools. They run 4 net
threads with server_max_connections 3, verify a pooled connection is reused and
migrated between threads, verify the request that hits the limit triggers
purge_keepalives() and succeeds, and check that
proxy.process.http.pooled_server_connections is back to 0 at the end.
##########
src/proxy/http/HttpSessionManager.cc:
##########
@@ -356,24 +400,72 @@ ServerSessionPool::eventHandler(int event, void *data)
return 0;
}
+void
+ServerSessionPool::deferClose(PoolableSession *ssn)
+{
+ // The session is not tracked by a pool anymore, so nothing can be allowed
to fire on it before
+ // it is closed.
+ ssn->do_io_read(nullptr, 0, nullptr);
+ ssn->do_io_write(nullptr, 0, nullptr);
+ ssn->cancel_inactivity_timeout();
+ ssn->cancel_active_timeout();
+
+ if (m_deferred_close.empty()) {
+ this_ethread()->schedule_imm_local(this);
+ }
+ m_deferred_close.push_back(ssn);
+}
+
+void
+ServerSessionPool::closeDeferred()
+{
+ // Closing a session can run plugin hooks, so take the list in case that
ends up adding to it.
+ std::vector<PoolableSession *> sessions;
+
+ sessions.swap(m_deferred_close);
+ closeSessions(sessions);
+}
+
void
HttpSessionManager::init()
{
m_g_pool = new ServerSessionPool;
eventProcessor.schedule_spawn(&initialize_thread_for_http_sessions, ET_NET);
}
-// TODO: Should this really purge all keep-alive sessions?
-// Does this make any sense, since we always do the global pool and not the
per thread?
+// Only the shared pool is purged, not the per thread pools.
+//
+// A session is closed by the thread that owns its connection, so this asks
every thread to close
+// its own. That keeps the closes out of the pool lock and away from other
threads' connections.
void
HttpSessionManager::purge_keepalives()
{
- EThread *ethread = this_ethread();
+ auto const &group = eventProcessor.thread_group[ET_NET];
- MUTEX_TRY_LOCK(lock, m_g_pool->mutex, ethread);
- if (lock.is_locked()) {
- m_g_pool->purge();
- } // should we do something clever if we don't get the lock?
+ for (int i = 0; i < group._count; ++i) {
+ if (EThread *thread = group._thread[i]; thread != nullptr &&
thread->server_session_pool != nullptr) {
+ thread->server_session_pool->requestPurge(thread);
Review Comment:
Both done in f068f1f95. The pool now remembers its owning thread from
initialize_thread_for_http_sessions and requestPurge() takes no argument. The
timing change is described in that commit message and I added it to the PR
description too. Note the current thread now closes its own pooled sessions
before purge_keepalives() returns; without that the request which hit the limit
ran out of retries before any purge event ran.
##########
src/proxy/http/HttpSessionManager.cc:
##########
@@ -285,6 +313,17 @@ ServerSessionPool::eventHandler(int event, void *data)
PoolableSession *s = nullptr;
switch (event) {
+ case EVENT_IMMEDIATE:
+ case EVENT_INTERVAL:
+ if (m_purge_requested.exchange(false) &&
!httpSessionManager.purge_thread_keepalives()) {
Review Comment:
Added the comment in f068f1f95: both requestPurge() and deferClose()
schedule the pool and an event from either does whatever work is pending.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
