JuliaLang / MbedTLS.jl

Wrapper around mbedtls
Other
41 stars 50 forks source link

Add GC-safe regions around `ssl_read` and `ssl_write` #265

Closed kpamnany closed 10 months ago

kpamnany commented 11 months ago

We've observed mbedtls_ssl_read and mbedtls_ssl_write show up in CPU profiles while also seeing long GC time-to-safepoint. These ccalls call back into Julia (f_send and f_recv), during which GC should be able to run. But I can't tell if, for large enough reads or writes, there could be a long interval without reaching a GC safepoint.

Adding GC-safe regions around the ccalls will let GC run even if a thread is busy inside. Since c_send and c_recv are @cfunctions, they will do the right thing wrt GC safety.

Ref: https://github.com/JuliaLang/julia/issues/51574

This is a temporary fix; when https://github.com/JuliaLang/julia/pull/49933 lands, this should be changed to do whatever that requires.

If a reviewer knows for certain that a thread won't remain inside the ccall without calling back into Julia for multiple seconds, then this PR can be discarded.

codecov[bot] commented 11 months ago

Codecov Report

Merging #265 (9d9a5a5) into master (e84f973) will increase coverage by 0.36%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #265      +/-   ##
==========================================
+ Coverage   73.49%   73.86%   +0.36%     
==========================================
  Files          12       12              
  Lines         732      750      +18     
==========================================
+ Hits          538      554      +16     
- Misses        194      196       +2     
Files Coverage Δ
src/ssl.jl 71.88% <100.00%> (+1.03%) :arrow_up:
kpamnany commented 10 months ago

The CI failures for Julia nightly are:

ERROR: LoadError: conversion to pointer not defined for Base.CodeUnits{UInt8, String}

They're coming from crypt! in cipher.jl here, which I haven't touched in this PR.

vtjnash commented 10 months ago

There are so many random pointer calls here that only serve to ensure incorrectness and serve no other value