bneradt commented on code in PR #13806:
URL: https://github.com/apache/trafficserver/pull/13806#discussion_r4276710019
##########
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:
`refresh()` re-checks `event_loop` after its `kevent` add and undoes the add
if a migration raced it. This level-triggered kqueue `modify()` path has the
same load/add window but no undo. It's probably not built anywhere in practice,
since kqueue is edge-triggered by default. Still, it might be worth either
matching the `refresh()` handling or adding a comment that this path is not
race-safe against `stop_for_migration()`.
##########
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:
These tests are all `TS_USE_KQUEUE`, so on Linux/epoll CI this file compiles
to nothing. The session pool restructuring itself has no coverage either: the
async purge, the deferred close of shared pool sessions, and the detach/attach
migration. Could you add an AuTest that runs with
`proxy.config.http.server_session_sharing.pool: global` (and `hybrid`) and
exercises cross-thread reuse? One that also sets
`proxy.config.http.server_max_connections` low enough to trigger
`purge_keepalives()` would cover the new purge path, and it could check that
`proxy.process.http.pooled_server_connections` returns to 0 afterward, which is
the gauge-drift fix. If it fits, `AtsReplayTest` with a replay file would keep
it short. Otherwise the `Test` class style in `ats_probe.test.py` works well.
##########
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:
Small thing: the deferred-close event and the purge event both come through
this case, so a deferred-close event can end up doing a pending purge early,
and the purge's own event then does nothing. That's harmless, but a short
comment saying both producers share this handler on purpose would help the next
reader.
##########
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:
Before this change the return value of `ep.stop()` was ignored. Now a failed
`epoll_ctl(EPOLL_CTL_DEL)` or `kevent(EV_DELETE)` aborts the process. On epoll,
`stop()` can return -1/ENOENT when `event_loop` is set but the fd was already
removed from the set. That can happen in a level-triggered build, where
`modify()` issues `EPOLL_CTL_DEL` once the events reach zero. Would logging and
treating this as a migration failure be enough here, maybe `Error()` plus
`origin_shutdown_migration_failure`? A failed delete on a pooled origin
connection doesn't seem worth taking down the host for. If you want to keep the
assert, an `ink_assert` for debug builds plus a runtime fallback would still
catch it in CI.
##########
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:
Behavior note: `purge_keepalives()` used to free connections synchronously
on the calling thread (when it got the lock). Now it only schedules work. The
request in `HttpSM` that triggers it fails either way, so that doesn't change.
But for a while after hitting `server_max_connections`,
`current_server_connections` will stay at the limit until every net thread runs
its event, so a few more requests may get ENFILE than before. That seems fine,
but could you mention it in the commit message? Also nit:
`requestPurge(thread)` passes the thread that already owns the pool. A pool
could just remember its owning thread in `initialize_thread_for_http_sessions`
instead of taking it as an argument.
--
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]
