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

Close chan of Closer first before calling callback #14231

Merged
merged 3 commits into from
Nov 14, 2019

Conversation

vjsamuel
Copy link
Contributor

Calling the callback first before closing the channel is causing read from closed connection exceptions. Ideally the processing should stop first before calling Close() on the tcp connection.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

1 similar comment
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@ph ph self-requested a review October 25, 2019 13:46
@ph ph added the review label Oct 25, 2019
@ph
Copy link
Contributor

ph commented Oct 25, 2019

@vjsamuel Looking at the code of the TCP Input, I think you are right if we move the close of the channel. Now Looking at the TCP input code

if err != nil {
// we are forcing a Close on the socket, lets ignore any error that could happen.
select {
case <-closer.Done():
break
default:

The select will also need to happen even if we don't get an error for this to works as expected.
Right now the callback is called which force a close on the TCP connection and make the Read() return an error and then the select is executed.

Now by closing the channel before the callback the following events will make the the handle stop:

  • Either read return and errors and Done() is checked.
  • Either the read hit the timeout deadl and Done() is checked
  • Either I successfully read a message and Done() is checked.

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

should have done the above comment with request changed.

close(c.done)

// TODO: callbacks that do socket closing would need to somehow synchronize the finish
// TODO: of Handle() and then do the close to avoid reading from closed connection.
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets remove the TODO here.

@vjsamuel vjsamuel force-pushed the tcp_close branch 2 times, most recently from 495637a to 549a4aa Compare October 25, 2019 18:12
Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

Remove the comment and LGTM,

@@ -94,15 +94,25 @@ func (c *splitHandler) Handle(closer CloseRef, conn net.Conn) error {
//16 is ratio of MaxScanTokenSize/startBufSize
buffer := make([]byte, c.maxMessageSize/16)
scanner.Buffer(buffer, int(c.maxMessageSize))
for scanner.Scan() {
for {
// we are forcing a Close on the socket, lets ignore any error that could happen.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment doesn't fit here, lets remove it.

@exekias exekias merged commit f944680 into elastic:master Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants