Skip to content
This repository was archived by the owner on Oct 9, 2023. It is now read-only.

Commit 03a0340

Browse files
authored
Remove calls to grpc.channel.close(), which could cause segfaults (#266)
## What is the goal of this PR? We no longer call `close` on any of our gRPC Channels. This fixes possible segfaults caused by resources being deallocated while they are still in use. ## What are the changes implemented in this PR? Users in a wide variety of scenarios had reported intermittent crashes, often accompanied by warnings saying "1 metadata element(s) leaked". The logs would look similar to the following: ``` 7786| E1031 17:59:35.477338904 207 metadata.cc:253] WARNING: 1 metadata elements were leaked 7787| E1031 17:59:35.477409424 207 metadata.cc:260] mdelem ':authority' = 'typedb-cluster-1.typedb-cluster:1729' 7788| [mutex.cc : 435] RAW: Lock blocking 0x563cbf829fc0 @ 7789| [mutex.cc : 1908] RAW: Check (v & (kMuWait | kMuWrWait)) != kMuWrWait failed: Lock: Mutex corrupt: waiting writer with no waiters: 0x563cbf815670 ``` The same issue has also been reported in googleads/google-ads-python#384, and a fix was suggested in: - grpc/grpc#19235 From this issue we infer that `Channel.close` is not behaving nicely in gRPC Python, and can cause resources to be deallocated while they are still in use. As gRPC itself uses native C libraries, this results in segfaults and crashes. We determine that the best course of action is to not close the Channel ourselves. We've simply deleted the 3 places in our code that called `close` on a gRPC `Channel`. It has passed our CI tests and fixed user-reported issues, and it is what the gRPC maintainers themselves appear to recommend in the linked `grpc` issue. - closes #239 - see #265
1 parent a850b2e commit 03a0340

File tree

7 files changed

+90
-136
lines changed

7 files changed

+90
-136
lines changed

.grabl/automation.yml renamed to .factory/automation.yml

Lines changed: 89 additions & 132 deletions
Large diffs are not rendered by default.
File renamed without changes.
File renamed without changes.

BUILD

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ checkstyle_test(
5050
include = glob([
5151
".bazelrc",
5252
".gitignore",
53-
".grabl/*",
53+
".factory/*",
5454
"BUILD",
5555
"WORKSPACE",
5656
"deployment.bzl",

typedb/connection/cluster/server_client.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,6 @@ def _new_channel(self) -> grpc.Channel:
6363

6464
def close(self) -> None:
6565
super().close()
66-
self._channel.close()
6766

6867

6968
class _CredentialAuth(grpc.AuthMetadataPlugin):

typedb/connection/core/client.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,4 +49,3 @@ def new_channel_and_stub(self) -> (Channel, _CoreStub):
4949

5050
def close(self) -> None:
5151
super().close()
52-
self._channel.close()

typedb/connection/transaction.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,6 @@ def rollback(self):
101101

102102
def close(self):
103103
self._bidirectional_stream.close()
104-
self._channel.close()
105104

106105
def __enter__(self):
107106
return self

0 commit comments

Comments
 (0)