Preserve original fd blocking state in TLS I/O operations#1298
Preserve original fd blocking state in TLS I/O operations#1298ranshid merged 1 commit intovalkey-io:unstablefrom
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1298 +/- ##
============================================
+ Coverage 70.69% 70.70% +0.01%
============================================
Files 114 115 +1
Lines 63161 63185 +24
============================================
+ Hits 44650 44678 +28
+ Misses 18511 18507 -4
|
xbasel
force-pushed
the
restoreBlockingModeOnTls
branch
from
a2e1b22 to
f69607a
Compare
|
Please note that timeouts are still not restored. Not sure we should worry about timeouts. |
ranshid
left a comment
ranshid
left a comment
There was a problem hiding this comment.
I agree that the timeout is not the thing we should keep track of, however I wonder why we cannot just flag the connection with indication that it is blocked instead of going through the fcntl all the time?
Using an app-level flag adds complexity and risks desync with the kernel. fcntl is lightweight, reliable, and avoids duplication. |
|
@xbasel can you please signoff your commit? Also please elaborate more on the top comment. provide some example etc... |
This change prevents unintended side effects on connection state and improves consistency with non-TLS sync operations. Signed-off-by: xbasel <[email protected]>
xbasel
force-pushed
the
restoreBlockingModeOnTls
branch
from
f69607a to
aa0db5c
Compare
2 participants
This change prevents unintended side effects on connection state and improves consistency with non-TLS sync operations.
For example, when invoking
connTLSSyncReadwith a blocking file descriptor, the mode is switched to non-blocking uponconnTLSSyncReadexit. If the code assumes the file descriptor remains blocking and calls the normalreadexpecting it to block, it may result in a short read.This caused a crash in dual-channel, which was fixed in this PR by relocating
connBlock():#837