Skip to content

clickhouse-keeper: correctly handle closed client connection#35772

Merged
alesapin merged 3 commits intoClickHouse:masterfrom
azat:keeper-eof
Mar 31, 2022
Merged

clickhouse-keeper: correctly handle closed client connection#35772
alesapin merged 3 commits intoClickHouse:masterfrom
azat:keeper-eof

Conversation

@azat
Copy link
Member

@azat azat commented Mar 30, 2022

This will avoid noisy message like:

DB::Exception: Cannot read all data. Bytes read: 0. Bytes expected: 4.

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Cc: @alesapin

@azat
clickhouse-keeper: correctly handle closed client connection
9820230 
This will avoid noisy message like:

    DB::Exception: Cannot read all data. Bytes read: 0. Bytes expected: 4.

Signed-off-by: Azat Khuzhin <[email protected]>
@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Mar 30, 2022
@alesapin alesapin self-assigned this Mar 30, 2022
alesapin
alesapin reviewed Mar 30, 2022
View reviewed changes
Copy link
Member

@alesapin alesapin left a comment

Choose a reason for hiding this comment

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

🤔

azat added 2 commits March 30, 2022 21:30
@azat
clickhouse-keeper: fix implementation of server with poll()
38873d7 
Signed-off-by: Azat Khuzhin <[email protected]>
@azat
clickhouse-keeper: finish session on client EOF (fixes heap-use-after…
ff0e46c 
…-free)

CI founds [1]:

    WARNING: ThreadSanitizer: heap-use-after-free (pid=1)
      Read of size 8 at 0x7b48001e0088 by thread T7 (mutexes: write M2588):
        ...
        1 DB::KeeperTCPHandler::runImpl()::$_0::operator()() const build_docker/../src/Server/KeeperTCPHandler.cpp:384:14 (clickhouse+0x194f974f)
        ...
        8 DB::KeeperDispatcher::setResponse() build_docker/../src/Coordination/KeeperDispatcher.cpp:222:9 (clickhouse+0x1999de82)
        9 DB::KeeperDispatcher::responseThread() build_docker/../src/Coordination/KeeperDispatcher.cpp:161:18 (clickhouse+0x1999d7a7)
        10 DB::KeeperDispatcher::initialize()::$_1::operator()() const build_docker/../src/Coordination/KeeperDispatcher.cpp:273:54 (clickhouse+0x199a55d1)

      Previous write of size 8 at 0x7b48001e0088 by thread T3:
        0 operator delete(void*, unsigned long) <null> (clickhouse+0xa0dad50)
        1 DB::KeeperTCPHandler::~KeeperTCPHandler() build_docker/../src/Server/KeeperTCPHandler.cpp:648:1 (clickhouse+0x194f8ee6)

  [1]: https://s3.amazonaws.com/clickhouse-test-reports/35772/38873d726d22b15d4c5a284410956927b2e2f95f/integration_tests__thread__actions__[4/4].html

Signed-off-by: Azat Khuzhin <[email protected]>
@azat
Copy link
Member Author

azat commented Mar 31, 2022

Stateless tests (aarch64, actions) — fail: 1, passed: 3937, skipped: 15

2022-03-30 16:25:42 environment: line 1: wait_for: No record of process 212040

Integration tests (thread, actions) [4/4] — fail: 5, passed: 574, flaky: 5

WARNING: ThreadSanitizer: heap-use-after-free (pid=1)
  Read of size 8 at 0x7b48001e0088 by thread T7 (mutexes: write M2588):
    ...
    1 DB::KeeperTCPHandler::runImpl()::$_0::operator()() const build_docker/../src/Server/KeeperTCPHandler.cpp:384:14 (clickhouse+0x194f974f)
    ...
    8 DB::KeeperDispatcher::setResponse() build_docker/../src/Coordination/KeeperDispatcher.cpp:222:9 (clickhouse+0x1999de82)
    9 DB::KeeperDispatcher::responseThread() build_docker/../src/Coordination/KeeperDispatcher.cpp:161:18 (clickhouse+0x1999d7a7)
    10 DB::KeeperDispatcher::initialize()::$_1::operator()() const build_docker/../src/Coordination/KeeperDispatcher.cpp:273:54 (clickhouse+0x199a55d1)

  Previous write of size 8 at 0x7b48001e0088 by thread T3:
    0 operator delete(void*, unsigned long) <null> (clickhouse+0xa0dad50)
    1 DB::KeeperTCPHandler::~KeeperTCPHandler() build_docker/../src/Server/KeeperTCPHandler.cpp:648:1 (clickhouse+0x194f8ee6)

Forgot KeeperDispatcher::finishSession

@azat
Copy link
Member Author

azat commented Mar 31, 2022 •
edited
Loading

AST fuzzer (MSan, actions) — Task failed: $?=1

By some reason is empty.

Stress test (thread, actions) — Fatal message in clickhouse-server.log (see fatal_messages.txt)

Came to the same fix, but after rebase the fix was lost, because it had been fixed in a byte-identical way in upstream already:)

@alesapin alesapin merged commit bd2ab32 into ClickHouse:master Mar 31, 2022
@azat azat deleted the keeper-eof branch March 31, 2022 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alesapin alesapin alesapin approved these changes

Assignees

@alesapin alesapin

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@azat @alesapin @robot-ch-test-poll1