Comments (22)
You can only transfer an idle session (one with no transactions).
from proxygen.
Right, but i assume this is coming from ServerIdleSessionControlller::GetIdleSession() . Why would popBestIdlePool() allow it to try to transfer a session with non zero amount of txn's?
from proxygen.
Presumably, could it have gotten a transaction inbetween PopBestIdlePool() and the call to DetachThreadLocals() https://github.com/facebook/proxygen/blob/main/proxygen/lib/http/connpool/ServerIdleSessionController.cpp#L38C27-L38C27
from proxygen.
I think that should be prevented because removeOldestIdleSession should return nullptr if the pool has no more idle sessions.
from proxygen.
Is ServerIdleSessionController threadSafe though? It seems like we have one to many relationship with SessionPool? Would wrapping it in a folly::Synchronized be non performant?
I am confused why this would occur otherwise. If many threads are trying to transfer the same session, or its original thread is getting a transaction on that session while other threads are accessing it, it seems plausible these conditions can be met.
from proxygen.
Im successfully transferring threads in my production tests, but am still experiencing this crash occasionally (under moderate load).
from proxygen.
It seems to me: PopBestIdlePool() prevents any access from the serverIdleSessionController on another thread, but does not prevent access to that idle session via that sessions current session Pool.
from proxygen.
We've run this code in prod for many years under all kinds of load scenarios and don't see this crash, so something must be different.
Your application code needs to make the session not idle (by calling newTransaction) immediately after the future is fulfilled, or a different thread can grab that session again.
We've been cooking up a C++ coroutine version of the proxygen library (watch this space!) which addresses this problem by having the session pool code make a "reservation" for a future request before returning a session, which makes it not idle.
from proxygen.
Understood. I am sure there is something different somewhere, but i am still confused how its possible that transactions can be non-empty since this is all happening within the control flow of ServerIdleSessionController. The session in question is on the idle pool, but by the time detachThreadLocals is called, it has a transaction..
After the future returns I call newTransaction immediately, yes.
from proxygen.
Yeah I'm pretty confused too. Seems like maybe somehow the same session is being used in multiple threads - such that between "removeOldestIdleSession" and "detachTheadLocals" the other thread creates a transaction. I can't really reason how that happened though, unless you put it back in multiple pools, or put it in the pool while retaining a reference to it that you are continuing to manipulate.
from proxygen.
Something like that makes sense. I am going to try and use a local build of proxygen with my own logging to suss out the error. Do you have any tips of places I can add logging? Perhaps to see if a session belongs to multiple pools
from proxygen.
Just -v 3 logging should log the session pointer value and the thread ID. -v 4 is a good trace of session but will crumble and affect timing under load.
Otherwise, instrument some of the calls in connpool/ like putSession, getTransaction, getIdleSession, ...
from proxygen.
Where can I set that flag? We use cmake via bazel to build proxygen.
from proxygen.
It's a runtime flag to your binary, assuming it calls folly::init or gflags/glog initializers.
from proxygen.
Right yeah we can do that thanks. Is -v=3 still the convention? I see here #409 that we should use something like --logging=DBG2?
from proxygen.
Oh we're halfway between switching between glog and folly logging. I think glog (-v) still covers most of the tracing, but you can use both options too.
from proxygen.
From the logs I can confirm that I am placing a session(1) on a pool, and immediately put a transaction on this session.
10 ms later, another thread attempts to transfer a session and the aforementioned session is chosen (but has a transaction with the same address, which is logged in detachThreadLocals()).
Why would this session be marked as Idle? Could it be an issue with the serverIdleSessionController?
from proxygen.
Is it possible calling newTransaction() on a session directly after putting it in the pool (instead of putting the session in the pool, and then calling getTransaction on the pool) is the root cause?
from proxygen.
It seems to have fixed it - but i am wondering what the correct convention is here (its not documented).
Should I always call getTransaction on the pool instead of calling newTransaction on the session itself? There are two cases:
- when we get a session from http::connector callback (connect)
- when we get a session from thread transfer
Changing 1) to call getTransaction stopped the crashes, even though in 2) we call newTransaction()
from proxygen.
Is it possible calling newTransaction() on a session directly after putting it in the pool (instead of putting the session in the pool, and then calling getTransaction on the pool) is the root cause?
Yes. If you do this:
connectSuccess(session) {
pool->putSession(session);
auto txn = session->newTransaction();
}
Then there is a potential race, where another thread takes session out of the pool and wants to start transferring it to its own thread. Then newTransaction
makes it non-idle, which blows up detachThreadLocals
.
Our code does it this way:
connectSuccess(session) {
auto txn = session->newTransaction();
pool->putSession(session);
}
Now the session goes into the pool non-idle, and won't be touched by other threads. What you are doing:
connectSuccess(session) {
pool->putSession(session);
auto txn = pool->getTransaction();
}
Is somewhat different. It's thread-safe, but there's a chance you lose the race. Another thread can pop the new session out of the pool, and txn could be nullptr.
from proxygen.
I'll also add, the new coroutine version of the session pool encapsulates the connection establishment step, so you can't make this kind of error. Sorry this subtlety tripped you up for so long.
from proxygen.
Understood! Makes sense. Thanks for all the help.
from proxygen.
Related Issues (20)
- Bandwidth Utilization
- This resolves CVE-2023-44487.
- Building Proxygen in Windows
- HTTP3 Server and Client from samples HOT 11
- Does proxygen support iOS ?
- Transferring Sessions between threads for Http Client Connection Pool HOT 8
- Homebrew build of proxygen failing HOT 7
- ServerIdleSessionConroller:getIdleSession() returns session already in thread HOT 5
- Concurrent performance supported by the HTTP server
- Websocket Key Mismatch HOT 2
- Connection Close Reason SHUTDOWN HOT 3
- `EchoHandler` in `proxygen/httpserver/samples/echo/EchoHandler.h` has unnecessary `body_` member
- Request smuggling vulnerability in Proxygen
- websocket client with SSL problem: Server replied to me "Bad Request" HOT 5
- proxygen fails to build when a newer glog package is installed
- Proxygen for iOS HOT 2
- httpserver/samples/websocket
- Ingress Error and drain() HOT 2
- httpserver/samples/websocket
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from proxygen.