Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Main Thread Panic when swarmed with clients #158

Merged
merged 10 commits into from
Sep 5, 2022

Conversation

drdrsh
Copy link
Collaborator

@drdrsh drdrsh commented Sep 5, 2022

thread 'main' panicked at 'attempt to add with overflow', src/main.rs:289:17
stack backtrace:
   0: rust_begin_unwind
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/panicking.rs:142:14
   2: core::panicking::panic
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/panicking.rs:48:5
   3: pgcat::main::{{closure}}
             at ./src/main.rs:289:17
   4: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/future/mod.rs:91:19
   5: tokio::park::thread::CachedParkThread::block_on::{{closure}}
             at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.16.1/src/park/thread.rs:263:54
   6: tokio::coop::with_budget::{{closure}}
             at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.16.1/src/coop.rs:102:9
   7: std::thread::local::LocalKey<T>::try_with
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/thread/local.rs:445:16
   8: std::thread::local::LocalKey<T>::with
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/thread/local.rs:421:9
   9: tokio::coop::with_budget
             at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.16.1/src/coop.rs:95:5
  10: tokio::coop::budget
             at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.16.1/src/coop.rs:72:5
  11: tokio::park::thread::CachedParkThread::block_on
             at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.16.1/src/park/thread.rs:263:31
  12: tokio::runtime::enter::Enter::block_on
             at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.16.1/src/runtime/enter.rs:151:13
  13: tokio::runtime::thread_pool::ThreadPool::block_on
             at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.16.1/src/runtime/thread_pool/mod.rs:73:9
  14: tokio::runtime::Runtime::block_on
             at ~/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.16.1/src/runtime/mod.rs:477:43
  15: pgcat::main
             at ./src/main.rs:298:5
  16: core::ops::function::FnOnce::call_once
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/ops/function.rs:248:5

@zain-kabani
Copy link
Contributor

We might want to consider setting max client connections at some point

@drdrsh drdrsh marked this pull request as ready for review September 5, 2022 01:46
@@ -95,7 +95,7 @@ pub async fn client_entrypoint(
mut stream: TcpStream,
client_server_map: ClientServerMap,
shutdown: Receiver<()>,
drain: Sender<i8>,
drain: Sender<i32>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, I forgot we're keeping this one alive until the client disconnects. Good catch. I'd make it a usize to avoid this problem forever.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought....we're only sending 1 and 0 through this channel. Why would it overflow?

Copy link
Collaborator

@levkk levkk Sep 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, because total_clients becomes an i8. Wow! Subtle.

src/client.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@levkk
Copy link
Collaborator

levkk commented Sep 5, 2022

We might want to consider setting max client connections at some point

That limit ended up always being a file descriptor issue, not anything to do with the proxy itself. All of our data structures should be O(1), so number of clients shouldn't matter.

@levkk levkk merged commit 976b406 into postgresml:main Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants