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.

Sign up for GitHub

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

Jump to bottom

Adds block property to socket io results #13409

Closed
wants to merge 7 commits into from
Closed

Adds block property to socket io results #13409

wants to merge 7 commits into from

Conversation

davinci26
Copy link
Member

Signed-off-by: Sotiris Nanopoulos sonanopo@microsoft.com

In preparation for #13189 we add the following functionality to the internal API:

  1. getEnabled to file_event.h which returns the active events in a file event. This is useful to
  2. didBlock to IoResult to know if the last I/O call blocked.

Risk Level: Low
Testing: Tests
Docs Changes: N/A
Release Notes: N/A

Adds block property for socket io results
9e2ab36 
Signed-off-by: sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26
Copy link
Member Author

davinci26 commented Oct 6, 2020 •
edited
Loading

Why this change?

The way we plan to use this functionality is the following:

We want in connection_impl, If write blocks, add write to the registration mask or add a write activation event (for edge events) and similarly for if read blocks, add read to the registration mask if read blocks.

Note on the implementation

@antoniovicente I know that we discussed adding this functionality to io_handle but I am not sure if this is a good idea. Because there is a one-to-many relationship between the file_event and io_handle i.e. we can have multiple file_events in a single io_handle.

This makes it hard when we call writev to know which file_event it corresponds to and update the registration only for that. This becomes even trickier because the ownership semantics of file_event (unique ptr) which makes it clear that file_event is owned by the consumer.

cc @envoyproxy/windows-dev

fix cascading build breaks
107ed7a 
Signed-off-by: sotiris Nanopoulos <sonanopo@microsoft.com>
Sotiris Nanopoulos added 2 commits October 6, 2020 10:08
fix proxy protocol
949914a 
Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>
format change
d36b5f9 
Signed-off-by: sotiris Nanopoulos <sonanopo@microsoft.com>
@davinci26 davinci26 changed the title Adds block property for socket io results Adds block property to socket io results Oct 6, 2020
@nigriMSFT
Copy link
Contributor

Do we have a test for the would_block status flag? Something analogous to TEST_P(UdpListenerImplTest, SendDataError) where we request IO, the underlying syscall returns EWOULDBLOCK, and we verify that the IoResult has the would_block flag set.

@davinci26
Copy link
Member Author

davinci26 commented Oct 6, 2020 •
edited
Loading

Do we have a test for the would_block status flag? Something analogous to TEST_P(UdpListenerImplTest, SendDataError) where we request IO, the underlying syscall returns EWOULDBLOCK, and we verify that the IoResult has the would_block flag set.

Agreed will add some tests to verify the property added. Let me see all the cascading changes in all the socket types and I will add a test for each socket that I have changed

sotiris Nanopoulos added 3 commits October 6, 2020 13:31
fixes file_event and pr comment
c7b2024 
Signed-off-by: sotiris Nanopoulos <sonanopo@microsoft.com>
rename from didBlock to would_block
f36dad1 
Signed-off-by: sotiris Nanopoulos <sonanopo@microsoft.com>
more CI work
464d705 
Signed-off-by: sotiris Nanopoulos <sonanopo@microsoft.com>
nigriMSFT
nigriMSFT reviewed Oct 7, 2020
View reviewed changes
include/envoy/network/transport_socket.h
/**
* True if the I/O call would block.
*/
bool would_block;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would the convention be would_block_?

nigriMSFT
nigriMSFT reviewed Oct 7, 2020
View reviewed changes
source/common/network/raw_buffer_socket.cc
@@ -16,6 +16,7 @@ void RawBufferSocket::setTransportSocketCallbacks(TransportSocketCallbacks& call
IoResult RawBufferSocket::doRead(Buffer::Instance& buffer) {
PostIoAction action = PostIoAction::KeepOpen;
uint64_t bytes_read = 0;
bool wouldBlock = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would the convention be would_block?

Copy link
Member Author

Choose a reason for hiding this comment

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

ack, I think you are right again 😄

nigriMSFT
nigriMSFT reviewed Oct 7, 2020
View reviewed changes
source/common/network/raw_buffer_socket.cc
@@ -38,18 +39,20 @@ IoResult RawBufferSocket::doRead(Buffer::Instance& buffer) {
ENVOY_CONN_LOG(trace, "read error: {}", callbacks_->connection(),
result.err_->getErrorDetails());
if (result.err_->getErrorCode() != Api::IoError::IoErrorCode::Again) {
wouldBlock = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is still wrong. Shouldn't we set would_block to true if (result.err_->getErrorCode() == Api::IoError::IoErrorCode::Again)

Copy link
Member Author

Choose a reason for hiding this comment

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

I am trying to set up tests for all the sockets so we can make sure that we got all of them correctly

@davinci26
Copy link
Member Author

Based on the discussion on slack channel https://envoyproxy.slack.com/archives/C78HA81DH/p1602261758081000
fd and file_events_ have a 1-1 to mapping as a result this PR is not the best way to go forward.

Closing in favour of #13478

@davinci26 davinci26 closed this Oct 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nigriMSFT nigriMSFT nigriMSFT left review comments

@lizan lizan Awaiting requested review from lizan

@PiotrSikora PiotrSikora Awaiting requested review from PiotrSikora

@alyssawilk alyssawilk Awaiting requested review from alyssawilk

@htuch htuch Awaiting requested review from htuch

Assignees

@antoniovicente antoniovicente

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@davinci26 @nigriMSFT @antoniovicente