Skip to content

add "Proper memory safety with GC.@preserve" #204

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

Closed
wants to merge 8 commits into from

Conversation

kashish2210
Copy link

Add GC.@preserve and enhance table string operations for Julia nightly compatibility

Description

This PR addresses the Julia nightly compatibility issues of this #196 highlighted in #194 by improving string handling in table operations and ensuring proper memory management. Key improvements include the addition of GC.@preserve blocks and a new safe_table_string_convert function.

Changes

1. String Handling Improvements

Added new safe_table_string_convert function for safer string handling in table operations:
The below function was initiated in #196 I have updated a lit bit:)

function safe_table_string_convert(str::AbstractString)
    GC.@preserve str begin
        # Ensure string data remains valid during conversion
        converted = # string conversion logic
        return converted
    end
end

Implemented type traits pattern to prevent method overwriting:

abstract type VarColHandler end
struct StringVarColHandler <: VarColHandler end
struct NumericVarColHandler <: VarColHandler end
struct UnsupportedVarColHandler <: VarColHandler end

Copy link

codecov bot commented Mar 29, 2025

Codecov Report

Attention: Patch coverage is 81.36201% with 52 lines in your changes missing coverage. Please review.

Project coverage is 86.85%. Comparing base (52a53d1) to head (45e54d4).
Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
src/table.jl 67.52% 38 Missing ⚠️
src/fits.jl 89.24% 10 Missing ⚠️
src/header.jl 94.20% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #204      +/-   ##
==========================================
- Coverage   91.10%   86.85%   -4.25%     
==========================================
  Files           5        5              
  Lines         697      799     +102     
==========================================
+ Hits          635      694      +59     
- Misses         62      105      +43     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Convert a null-terminated byte buffer to string safely with bounds checking
and memory protection. Internal helper function.
"""
function safe_string_convert(x::Vector{UInt8})
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed? Why not using String(copy(x)) instead?

Copy link
Author

Choose a reason for hiding this comment

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

This function serves a specific purpose that String(copy(x)) doesn't handle:

  • Null Termination Handling

for example

# With null terminator
data = UInt8[65, 66, 67, 0, 68, 69]  # "ABC\0DE"

# Using String(copy(x)):
String(copy(data))  # Would give "ABC\0DE" (includes null and data after it)

# Using safe_string_convert:
safe_string_convert(data)  # Gives "ABC" (stops at null)

Comment on lines +1079 to +1083
# Add Windows-specific configuration at the start of the test file
if Sys.iswindows()
ENV["JULIA_CPU_THREADS"] = "1"
Base.GC.enable(true)
end
Copy link
Member

Choose a reason for hiding this comment

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

Why? ENV["JULIA_CPU_THREADS"] is also the wrong way to check the number of threads for two reasons:

  1. the environment variable may not be defined at all and you'd get an error in that case. To reference an env var which may not be defined you should use get(ENV, "JULIA_CPU_THREADS", <a default value here>)
  2. the right way to check the number of threads is Threads.nthreads()

Copy link
Author

Choose a reason for hiding this comment

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

i waill update this thanks for ur suggestion :)

if length(s) < 2 || s[1] != '\'' || s[end] != '\''
return nothing
end
GC.@preserve s begin
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to preserve s here?

Copy link
Author

Choose a reason for hiding this comment

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

this was unessary implmentaion let me clear it

@jishnub
Copy link
Member

jishnub commented May 22, 2025

The issue on master has been fixed now, so this PR should not be necessary anymore.

@jishnub jishnub closed this May 22, 2025
@kashish2210
Copy link
Author

No worries thanks for letting me know :)

@kashish2210 kashish2210 deleted the fix_issue#18 branch May 22, 2025 15:02
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.

3 participants