[poc] socket: mimic MSG_PEEK by read for windows#18305
[poc] socket: mimic MSG_PEEK by read for windows#18305soulxu wants to merge 3 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
| length = length - copy_size; | ||
| } else { | ||
| if (file_event_) { | ||
| file_event_->registerEventIfEmulatedEdge(Event::FileReadyType::Read); |
There was a problem hiding this comment.
In general it is assumed that it is a responsibility of the caller to re-active the event if they expect more data unless the error is WOULD_BLOCK
In the case of tls/proxy inspectors they should be able to determine that the data they currently have are not available to do their work and they should re-enable read. This is something that connection_impl does well.
There was a problem hiding this comment.
yea, same as the previous one, whether we should emulate all the cases. or we can just add assert here, assume people always read the data until WOULD_BLOCK return.
| } | ||
|
|
||
| Api::IoCallUint64Result IoSocketHandleImpl::recv(void* buffer, size_t length, int flags) { | ||
| if constexpr (Event::PlatformDefaultTriggerType == Event::FileTriggerType::EmulatedEdge) { |
There was a problem hiding this comment.
I am wondering if we should have a dedicated peek function for this.
| if (buffer_->length() > 0) { | ||
| auto move_length = std::min(buffer_->length(), max_length); | ||
| buffer.move(*buffer_); | ||
| if (file_event_) { |
There was a problem hiding this comment.
I am not sure if this is needed. The caller should be reading until EAGAIN is returned
There was a problem hiding this comment.
Emm...good point, should we emulate the case if the caller provides a small max_length?
|
@davinci26 thanks for the review! sorry for reply late, this week is holiday in China, I will back to work next week. |
|
@soulxu if you are busy let me know and I can take over this PR |
thanks! I almost done a version, I should be able to send out today. I should ask your help in the beginning, my windows dev env is on my laptop, and my laptop is too slow~, anyway let me push out this version. if there are more things to do, I will appreciate you can help it. |
|
Awesome no pressure, I just wanted to make sure this does not go stale because it is not high enough priority for you. Anytime, please involve me! |
In preparation for #18305, I split the implementation of io_socket_handle on different platforms to make the code more readable and separated. The new Windows class leverages the common implementation and now is responsible for registering the events. This will allow us to cleanly implement MSG_PEEK by read in a consequent PR in a self-contained way. Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
bot
added
the
stale
2 participants
Commit Message: socket: mimic MSG_PEEK by read for windows
Additional Description:
Windows only support level triggered event. The edge triggered event was emulated by the Envoy. But it depends on the envoy code read all the data out of socket buffer, then the
Readevent won't be triggered again. But for listener filter, we need to peek the data, the peek won't consume the data from the socket buffer, theReadevent will be triggered again, and becomes a busy loop.This was found in the PR below:
#17395 (comment)
This PR is trying to mimic the MSG_PEEK by a normal read.
Risk Level: high
Testing:
Docs Changes:
Release Notes: