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

BIO Stream can work with any IO (not just TCPSocket) #27

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

grlap
Copy link
Contributor

@grlap grlap commented May 10, 2023

on_bio_stream_read and on_bio_stream_write can use any IO (not just TCPSocket)
http server works fine

@quinnj
Copy link
Member

quinnj commented May 10, 2023

I restricted this to just TCPSocket because that's the only actual use I'm aware of and it helps w/ performance and avoiding a dynamic dispatch. Do you have a use-case other than tests where you're relying on using some other IO?

@grlap
Copy link
Contributor Author

grlap commented May 10, 2023

Self signed cert. Also writing and reading from the socket is just ugly.
What is the overhead of dynamic dispatch here ?

… bytesavailable

on_bio_stream_write return -1 if write fails (bss_sock.c)

This enables server side mode, however the logic in the bio still does not match what OpenSSL is doing.
@codecov-commenter
Copy link

codecov-commenter commented May 14, 2023

Codecov Report

Attention: Patch coverage is 79.82456% with 23 lines in your changes missing coverage. Please review.

Project coverage is 76.54%. Comparing base (3a20f19) to head (656473b).
Report is 5 commits behind head on main.

Files Patch % Lines
src/OpenSSL.jl 76.76% 23 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #27      +/-   ##
==========================================
- Coverage   76.80%   76.54%   -0.26%     
==========================================
  Files           2        2              
  Lines        1095     1113      +18     
==========================================
+ Hits          841      852      +11     
- Misses        254      261       +7     

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

@grlap grlap force-pushed the personal/grlap/write_bio branch from 0d40e60 to ee21d15 Compare May 18, 2023 20:58
@@ -420,7 +326,7 @@ macro geterror(ssl, op, expr)
esc(quote
# lock our SSLStream while we clear errors
# make a ccall, then check the error queue
Base.@lock ssl.lock begin
lock(ssl.lock) do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

otherwise macro fails (only Julia 1.6.7 Windows)

@grlap grlap changed the title Fix writing to BIO BIO Stream can work with any IO (not just TCPSocket) Jul 8, 2023
@mcvincekova
Copy link

@grlap any plan to merge this? :)

@grlap
Copy link
Contributor Author

grlap commented Jul 4, 2024

I'm still waiting for sign off.

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.

4 participants