Skip to content

Io socket handle for internal socket#13418

Closed
lambdai wants to merge 98 commits intoenvoyproxy:mainfrom
lambdai:bsock
Closed

Io socket handle for internal socket#13418
lambdai wants to merge 98 commits intoenvoyproxy:mainfrom
lambdai:bsock

Conversation

@lambdai
Copy link
Copy Markdown
Contributor

@lambdai lambdai commented Oct 7, 2020 •
edited
Loading

Close in favor of #14712 and #14917

Commit Message:
Introduce BufferedIoSocketHandleImpl and UserSpaceFileEventImpl.
This socket handle owns the buffer for itself to read data from and for the peer to write to.
UserSpaceFileEventImpl is the FileEvent this socket could work with.

UserSpaceFileEventImpl provides 3 functions

  1. setEnabled(events) declared by FileEvent. The events are registered for callback. Each call detects fired events once, and IO handle will notify the new events in the long run.
  2. activate(events). Explicitly activate the events. The above enabling events are not honored.
  3. poll(events). It should only be used by IO handle. Unlike activate, poll honors the setEnabled.

The BufferedIoSocketHandleImpl work with ConnectionImpl, and TcpListenerFilter
fdDoNotUse The high level socket option should be intercepted by ConnectionSocket
instead of accessing the fd.

Will provide the example of ConnectionImpl and ConnectionSocket in the next PR.

Additional Description:
Extracted from #13361
Risk Level: Low. Production is not using this socket.
Testing: Added unit test.
Docs Changes: N/A
Release Notes: N/A

lambdai added 4 commits October 6, 2020 20:53
@lambdai
format
f860594 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
amend
d90cbea 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
Merge remote-tracking branch 'origin/master' into bsock
80a9178 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
clean up and add impl ::read()
622ab92 
Signed-off-by: Yuchen Dai <[email protected]>
@alyssawilk
Copy link
Copy Markdown
Contributor

alyssawilk commented Oct 7, 2020

Please ping when this is ready for review.

@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Oct 8, 2020

@alyssawilk merged master and implement new socket api. Now the test should be passing and compatible with master...

@lambdai lambdai marked this pull request as ready for review October 8, 2020 21:34
lambdai added 2 commits October 8, 2020 22:55
@lambdai
remove posix header file
273447d 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
fix error code on read EOS
9f810f2 
Signed-off-by: Yuchen Dai <[email protected]>
alyssawilk
alyssawilk reviewed Oct 12, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

Hey @lambdai, great to see this moving forward. Sending first pass comments your way

lambdai
lambdai commented Oct 12, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@lambdai lambdai left a comment

Choose a reason for hiding this comment

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

@alyssawilk Thanks! I will update the PR reflecting your comment!
However the pointers cannot be changed to references based on the current design(construct in pair). Maybe I am missing anything?

@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Oct 12, 2020

why default? isn't this more of a synthetic event handler?

Currently the default impl is weak.
I wish I can provide other implementation: level trigger, or better filtering events, and also support "FileEventType::Closed".

Alternatively the feature experimenting procedure can be done through improving DefaultEventListener.

@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Oct 12, 2020

why not closed? should we ASSERT it's not enabled?

I need a complicated EventListener implementation to supporting CLOSED event.
EventListener need to consume byte transmition status while read/write doesn't.

EV_CLOSED is not a MUST-have but a nice-to-have is that IIRC libevent only deliver EV_CLOSED if the poller is epoll.

I add the TODO for now

@lambdai
address comments
6422a4f 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
fix TestShutdown and close
a978ced 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
fix
0cf62b4 
Signed-off-by: Yuchen Dai <[email protected]>
yanavlasov
yanavlasov requested changes Oct 14, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@yanavlasov yanavlasov left a comment

Choose a reason for hiding this comment

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

Initial pass. There is a lot to digest here. Will continue tomorrow.

@lambdai
fix log and comments
09c1adf 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Oct 14, 2020

Adding test coverage for unsupported function... It shouldn't impact the normal flow

yanavlasov
yanavlasov requested changes Oct 14, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@yanavlasov yanavlasov left a comment

Choose a reason for hiding this comment

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

Will review the rest later today.

lambdai
lambdai commented Oct 14, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@lambdai lambdai left a comment

Choose a reason for hiding this comment

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

Updating commit at my earliest availability today

@lambdai
addressing comment
b73ced3 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
remove event counter
49bd8d6 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
more test and event counter fix
55b9acc 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Jan 13, 2021

@antoniovicente Sorry for the late reply! I am ramping up with the works left over.

I dontn't merge master but making it mergeable. You can still review the new commits as you did before.

@lambdai
massage merge master
29c1bea 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Jan 14, 2021

@antoniovicente
Add description of UserSpaceFileEventImpl so it's easier to review.

UserSpaceFileEventImpl provides 3 functions 
1. setEnabled(events) declared by FileEvent. The `events` are registered for callback. Each call detects fired events once, and IO handle will notify the new events in the long run.
2. activate(events). Explicitly activate the events. The above enabling events are not honored.
3. poll(events). It should only be used by IO handle. Unlike `activate`, poll honors the `setEnabled`.

@lambdai lambdai mentioned this pull request Jan 15, 2021
Merged
Base automatically changed from master to main January 15, 2021 23:01
antoniovicente
antoniovicente reviewed Jan 20, 2021
View reviewed changes
source/extensions/io_socket/buffered_io_socket/buffered_io_socket_handle_impl.cc Outdated
bytes_written += slices[i].len_;
}
}
writable_peer_->setNewDataAvailable();
Copy link
Copy Markdown
Contributor

@antoniovicente antoniovicente Jan 20, 2021

Choose a reason for hiding this comment

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

This call should be omitted if the buffer was not empty at the beginning of this method. We're looking for edge trigger behavior, no need to notify of new data if there was data in the buffer prior to the writev call.

ggreenway pushed a commit that referenced this pull request Feb 1, 2021
@lambdai
extension: User space event (#14712)
8e2286b 
UserSpaceFileEventImpl provides 3 functions

setEnabled(events) declared by FileEvent. The events are registered for callback. Each call detects fired events once, and IO handle will notify the new events in the long run.
activate(events). Explicitly activate the events. The above enabling events are not honored.
poll(events). It should only be used by IO handle. Unlike activate, poll honors the setEnabled. Will schedule callback if there is no pending callback.

Part of #13418

Signed-off-by: Yuchen Dai <[email protected]>
@antoniovicente
Copy link
Copy Markdown
Contributor

antoniovicente commented Feb 1, 2021

Needs main merge

@lambdai
Merge branch 'main' into bsock
7333584 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
fixing build
726e3e9 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
fix test build
4adf60c 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
fix build
1f91272 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
fix tests
65b590e 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Feb 2, 2021

Merged main branch and newly update API IoSocketHandleImpl::read has 1 line not covered :( Adding test

@lambdai
cover read(buf, 0)
8087b0d 
Signed-off-by: Yuchen Dai <[email protected]>
@lambdai lambdai mentioned this pull request Feb 2, 2021
Merged
@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Feb 2, 2021

/wait

@antoniovicente
Copy link
Copy Markdown
Contributor

antoniovicente commented Feb 17, 2021

I think this change is the same as #14917

Please close this PR.

@lambdai lambdai closed this Feb 17, 2021
@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Feb 17, 2021

Thank you @antoniovicente !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 left review comments

@yanavlasov yanavlasov yanavlasov left review comments

+3 more reviewers

@alyssawilk alyssawilk alyssawilk left review comments

@antoniovicente antoniovicente antoniovicente left review comments

@davinci26 davinci26 davinci26 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@yanavlasov yanavlasov

@alyssawilk alyssawilk

@antoniovicente antoniovicente

Labels

waiting

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@lambdai @alyssawilk @davinci26 @mattklein123 @yanavlasov @wrowe @antoniovicente