Skip to content

Conversation

dentiny
Copy link
Contributor

@dentiny dentiny commented Sep 27, 2025

This PR does two things:

  • (major) Assert libcurl operations' return value, otherwise it could fail silently
  • (major) Move header file inclusion out of duckdb namespace
  • (minor) Use atomic variable for httpfs client count, so we could (1) reduce one global variable; (2) slightly better perf

@dentiny dentiny changed the title Check libcurl return value [WIP] Check libcurl return value Sep 27, 2025
@dentiny dentiny marked this pull request as draft September 27, 2025 00:37
@dentiny dentiny force-pushed the hjiang/check-libcurl-return branch from fca51f5 to 3e98332 Compare September 27, 2025 00:38
@dentiny dentiny changed the title [WIP] Check libcurl return value Check libcurl return value Sep 27, 2025
@dentiny dentiny marked this pull request as ready for review September 27, 2025 00:59
@carlopi
Copy link
Collaborator

carlopi commented Sep 27, 2025

I think first point is valid, unsure if it should be a D_ASSERT (meaning only there on relassert) or then a throw of some kind (unsure which), so that is visible to all users.

Second split-off PR has already been merged.

Third I guess so.

@carlopi carlopi requested a review from Tmonster September 27, 2025 07:48
@dentiny
Copy link
Contributor Author

dentiny commented Sep 27, 2025

I think first point is valid, unsure if it should be a D_ASSERT (meaning only there on relassert) or then a throw of some kind (unsure which), so that is visible to all users.

The reason I use check failure here is: we're simply setting configs for curl handle, and they're not expected to fail.

@dentiny
Copy link
Contributor Author

dentiny commented Sep 30, 2025

Current implementation is actually buggy:

  • Under release debug mode, D_ASSERT is translated to assert, which does nothing
  • So to me, the best practice is never place any real logic inside of D_ASSERT

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.

2 participants