[native] Add check if session is reusable

prestodb / presto

The official home of the Presto distributed SQL query engine for big data

http://prestodb.io

Apache License 2.0

15.75k stars 5.28k forks source link

[native] Add check if session is reusable #23018

Closed majetideepak closed 2 weeks ago

majetideepak commented 2 weeks ago

Description

Check if the session is reusable before reusing it. Resolves https://github.com/prestodb/presto/issues/22995

Motivation and Context

Impact

Test Plan

Contributor checklist

[ ] Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
[ ] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
[ ] Documented new properties (with its default value), SQL syntax, functions, or other functionality.
[ ] If release notes are required, they follow the release notes guidelines.
[ ] Adequate tests were added if applicable.
[ ] CI passed.

== NO RELEASE NOTE ==

aditi-pandit commented 2 weeks ago

@majetideepak : From your comment here it seems like you didn't see any re-use after this fix. So is this ready for review ?

Don't want to regress anything internally in Meta if they are not encountering it.

@amitkdutta

majetideepak commented 2 weeks ago

@aditi-pandit I think this fix is reasonable given the session API documentation says isReusable must be true for a session to be reused. The current situation is bad since we are easily hitting the sigsegv in the OSS environment with large data. Since this is a blocking issue, I feel we should merge this and then look into the reuse issue.

Yuhta commented 2 weeks ago

It seems a bug in proxygen so let's fix it there instead. In the meantime anyone affected can disable connection pool.

majetideepak commented 2 weeks ago

The config to disable the connection pool is exchange.http-client.enable-connection-pool. I am closing this fix for now as it essentially disables the connection pool as well.

majetideepak commented 2 weeks ago

It seems a bug in proxygen so let's fix it there instead

@Yuhta, @amitkdutta It occurred to me that this PR helps overcome such bugs in proxygen. Maybe we should add a VELOX_CHECK(server->isReusable()) instead.

Yuhta commented 2 weeks ago

@majetideepak It will throw all the time and we will be forced to disable connection pool even we are not observing crashes before.

majetideepak commented 2 weeks ago

It will throw all the time and we will be forced to disable connection pool even we are not observing crashes before.

@Yuhta If I understood your comment here https://github.com/prestodb/presto/issues/22995#issuecomment-2176106700 correctly, isn't the expectation that the session->isReusable() should always be true? If that is not the case, is there any other precondition/check that we can add to avoid a crash?

majetideepak commented 2 weeks ago

Is isReusable buggy in Meta's setup too?

Yuhta commented 2 weeks ago

@majetideepak The value of isReusable is just our guess and it might not be aligned with how ServerIdleSessionController is supposed to do (our other internal usage of ServerIdleSessionController do not check isReusable either). So let's file the bug to expect the we don't get invalid sock_ from ServerIdleSessionController. Also did you run TSAN to make sure it's not some racing condition that caused this?

majetideepak commented 2 weeks ago

@czentgr ran with TSAN enabled. He did not find anything from that run.

majetideepak commented 2 weeks ago

isReusable is just our guess and it might not be aligned with how ServerIdleSessionController is supposed to do

Closing again!