morningman opened a new pull request, #69874:
URL: https://github.com/apache/doris/pull/69874
### What problem does this PR solve?
Issue Number: None
Related PR: #66331, #68353
Problem Summary:
**In short:** dropping or altering an ADBC catalog can crash the FE process.
Closing the catalog frees the native ADBC objects at once, including the
connections and results that other threads are still reading, such as the
column-statistics loads that a query on the catalog has just started. This
kills the FE in about one External regression run in 75, whatever the PR under
test changes. With this PR, closing a client only refuses new calls, and the
last call still in flight releases the driver as it leaves. DROP and ALTER
CATALOG still return without waiting.
This commit was first pushed as the last commit of #68353, which is still in
review. It shares nothing with the rest of that PR, so it is carved out here to
land on its own.
#### Background
An ADBC catalog (#66331) reads its metadata through `AdbcClient`, which owns
one native `AdbcDatabase` and one Arrow allocator per catalog. Every metadata
call borrows a fresh connection through `AdbcClient.withConnection` and closes
it afterwards. FE talks to the C driver through adbc-driver-jni 0.24. In that
bridge, closing a parent closes everything still open under it, on the closing
thread: `JniDatabase.close()` closes every connection still open on the
database, and `JniConnection.close()` closes every result (`getObjects` and
similar readers) still open on the connection. The bridge only checks a handle
before a call starts, and the C driver does not check at all, so freeing an
object that another thread is still using is a use-after-free in native code,
which takes the whole JVM down rather than failing one call.
Column statistics reach the driver too. When a query plans against an ADBC
table, the STATS_FETCH pool loads column statistics in the background. A load
that finds nothing in the statistics table asks the table itself:
`PluginDrivenExternalTable.getColumnStatistic` →
`AdbcConnectorMetadata.getTableHandle` → `withConnection` → `getObjects`. With
`"meta.cache.adbc.metadata.enable" = "false"`, every such load calls into the
driver.
#### The problem, and what it cost
- `AdbcClient.close()` closed the database and then the allocator
immediately, whatever was still in flight. DROP CATALOG and ALTER CATALOG ...
SET PROPERTIES close the catalog's connector this way, on their own thread
(since #69665, on the `ExternalResourceReclaimer` pool; before that,
synchronously on the statement thread, with the same race).
- A statistics load that was reading a `getObjects` result, or was inside
the driver on its connection, then read memory that had just been freed. Where
it dies depends on where that thread is at that moment.
- In CI, the External regression pipeline lost its FE in 11 of the 834 runs
between 2026-10-22 and 2026-10-30. Each time, 54 to 56 suites then failed with
`Communications link failure`, so the PR under test showed a red run it did not
cause. All 11 died at the same moment: `test_adbc_sqlite_catalog_scan` drops
its catalog with the metadata cache turned off right after querying it. In the
same millisecond, fe.log shows the allocator closing with `Memory was leaked by
query` and a statistics load failing with `Failed to read the ADBC getObjects
result: ArrowArrayStream is already closed`. That is the same race, caught by a
thread that came back only after the close had finished.
| FE crash in `fe.out` | Runs |
| --- | --- |
| `SIGSEGV` in
`nanoarrow::ArrayStreamFactory<VectorArrayStream>::get_next_wrapper+0x4` | 6 |
| `SIGSEGV` in `PrivateArrowSchemaDeepCopy+0x46` | 2 |
| glibc `double free or corruption (fasttop)` | 2 |
| no message | 1 |
| `SIGSEGV` in `sqlite3FindTable` (an earlier run, 2026-10-06) | 1 |
- Users can hit this too, not only CI: DROP or ALTER an ADBC catalog while a
statement or a statistics load is still reading it.
#### How this PR fixes it
- `AdbcClient` counts the calls inside `withConnection` (`enter` / `leave`,
under the client's monitor, which also opens the database on first use).
- `close()` marks the client closed, so new calls are refused with
`AdbcClient has been closed`. It releases the database and the allocator itself
only when no call is in flight. Otherwise it returns at once, and the last call
to leave releases them.
- A deferred release that fails is logged with the driver's path, as the
catalog already logs a failed connector close. It is not thrown at that last
caller, whose own work succeeded and which did not close anything.
- `close()` never waits for the calls in flight, so DROP and ALTER CATALOG
are not held up by a slow remote source.
#### Results
Unit tests, with the real JNI bridge and the SQLite driver (0 skipped):
| `AdbcClientTest` case | `close()` releasing at once, as before this PR |
This PR |
| --- | --- | --- |
| a call still on its connection when the client is closed keeps using it |
fails: `Native connection handle is closed` | passes |
| a call still reading a `getObjects` result when the client is closed reads
it to the end | fails: `Attempting operation on allocator when allocator is
closed` | passes |
| the last call to leave a closed client releases the database exactly once,
after it leaves | fails: `the database was released under an open connection` |
passes |
| a release that fails as the last call leaves does not fail that call |
fails: `the database was released under an open connection` | passes |
| an idle client releases at once | passes | passes |
A stress program (not committed) closes a client 200 times while four
threads loop over `getObjects` and `getTableSchema` through `withConnection`
against the SQLite driver:
| | `AdbcClient` of master | This PR |
| --- | --- | --- |
| Runs | 5 | 5 |
| JVM crashed | 5 of 5, 1.1 to 1.3 s after the JVM started: `SIGSEGV` in
`PrivateArrowSchemaDeepCopy+0x40`, under `ArrowArrayStreamReader.readSchema` ←
`AdbcObjectsReader.readNamespaces` ← `AdbcClient.withConnection` | 0 of 5 |
| Closes completed per run | crashed before finishing | 200 |
| Metadata calls per run | crashed before reporting | 6,019 to 7,238 |
| Calls the close reached while they were using the driver | | 0 |
| Failed closes, failed deferred releases | | 0, 0 (checked with WARN
logging on, where the unit test's deliberate release failure shows up as one
warning) |
The stress runs used the macOS arm64 build of the same SQLite driver, so the
offset differs from the Linux one in CI.
#### Classes, and how they call each other
- `AdbcClient`: `withConnection` brackets the body with `enter` / `leave`;
`close` refuses new calls and releases only when idle; `release` closes the
database, then the allocator; `openDatabase` is the one place the database is
created, so a test can stand a recording database in for the driver.
- `AdbcClientTest`: five new cases, two against the real driver and three
against a recording database.
```
STATS_FETCH: ColumnStatisticsCacheLoader.doLoad
-> PluginDrivenExternalTable.getColumnStatistic
-> AdbcConnectorMetadata.getTableHandle (namespaces, table names)
-> AdbcClient.withConnection
enter() refused once closed; callsInFlight++
-> AdbcConnection.getObjects ->
AdbcObjectsReader.readNamespaces / readTableNames
leave() callsInFlight--; the last one out of a closed client
-> release()
DROP CATALOG / ALTER CATALOG ... SET PROPERTIES
-> ExternalCatalog.onClose / resetToUninitialized ->
ExternalResourceReclaimer
-> AdbcConnector.close -> AdbcClient.close()
closed = true; release() now only if no call is in flight
release(): AdbcDatabase.close (closes its connections and their
results) -> allocator.close
```
Not changed: `AdbcConnector`, the catalog lifecycle and
`ExternalResourceReclaimer`, BE, and the regression suites;
`test_adbc_sqlite_catalog_scan` stays as it is, since what it exposes is this
bug.
### Release note
Fix an FE crash (SIGSEGV in the ADBC driver) when an ADBC catalog is dropped
or altered while a statement or a statistics load is still reading it.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- [x] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
Unit test: `AdbcClientTest`, 12 cases including the five new ones, and
all 197 tests of `fe-connector-adbc` pass with the real JNI bridge and SQLite
driver. The four new cases above fail with `close()` put back to releasing at
once. Manual: the stress program in the table above, run against the
`AdbcClient` of master and against this PR.
- Behavior changed:
- [x] No.
- [ ] Yes. <!-- Explain the behavior change -->
DROP and ALTER CATALOG still return without waiting. The native driver
of an ADBC catalog is now freed when its last in-flight call finishes, instead
of under it.
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
