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

fix(go/adbc/pkg): follow CGO rules properly #902

Merged
merged 3 commits into from
Jul 13, 2023

Conversation

lidavidm
Copy link
Member

Depends on apache/arrow#36670.

Fixes #729.

@lidavidm
Copy link
Member Author

lidavidm commented Jul 13, 2023

TODOs:

  • Update to upstream PR
  • Move the new test to be specific to the Go driver

@lidavidm lidavidm marked this pull request as ready for review July 13, 2023 20:22
@lidavidm lidavidm requested a review from zeroshade as a code owner July 13, 2023 20:22
@lidavidm
Copy link
Member Author

There's an apparent leak from the new test, will have to look

@lidavidm lidavidm added this to the ADBC Libraries 0.6.0 milestone Jul 13, 2023
Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

this all LGTM but i don't see any obvious issues that cause the leaked memory

@lidavidm
Copy link
Member Author

Weirdly enough, it's in an old test:

Note: Google Test filter = SqliteFlightSqlConnectionTest.MetadataGetTableTypes
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from SqliteFlightSqlConnectionTest
[ RUN      ] SqliteFlightSqlConnectionTest.MetadataGetTableTypes
[       OK ] SqliteFlightSqlConnectionTest.MetadataGetTableTypes (5 ms)
[----------] 1 test from SqliteFlightSqlConnectionTest (5 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (6 ms total)
[  PASSED  ] 1 test.

=================================================================
==1566443==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 128 byte(s) in 2 object(s) allocated from:
    #0 0x7f4c6193017a in __interceptor_calloc ../../../../libsanitizer/asan/asan_malloc_linux.cpp:154
    #1 0x7f4c611c0597 in _cgo_0a637ba8d569_C2func_calloc (/home/lidavidm/Code/upstream/adbc/adbc-crash/build/driver/flightsql/libadbc_driver_flightsql.so.6+0x988597)
    #2 0x7f4c60b34fc0 in runtime.asmcgocall.abi0 (/home/lidavidm/Code/upstream/adbc/adbc-crash/build/driver/flightsql/libadbc_driver_flightsql.so.6+0x2fcfc0)

SUMMARY: AddressSanitizer: 128 byte(s) leaked in 2 allocation(s).
fish: Job 1, './driver/flightsql/adbc-driver-…' terminated by signal SIGABRT (Abort)

@lidavidm
Copy link
Member Author

Ugh...moving the arrow-go version back fixes it. What happened...

@lidavidm
Copy link
Member Author

ah, I think I got it - PR incoming?

@lidavidm
Copy link
Member Author

Alright, I'm too frazzled to figure out the right thing today but since the other PR added a Retain() call on exported readers, we need to remember to Release() here now

@lidavidm
Copy link
Member Author

Actually no I just copy-pated a line twice, that's why my fix wasn't working

That should hopefully do it...

@lidavidm
Copy link
Member Author

And apache/arrow#36676 just for sanity

@lidavidm
Copy link
Member Author

hmm, my local clang-format seems to disagree with github all of a sudden...

@lidavidm lidavidm requested a review from zeroshade July 13, 2023 22:50
Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

LGTM awesome

@lidavidm lidavidm merged commit 7b1510a into apache:main Jul 13, 2023
65 checks passed
@lidavidm lidavidm deleted the gh-729-crash branch July 13, 2023 23:30
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.

python/adbc_driver_flightsql: crash in concurrent usage
2 participants