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

Convert an ink_release_assert into logic to reset the rbio to use the… #2147

Merged
merged 1 commit into from Jun 17, 2017

Conversation

shinrich
Copy link
Member

… remaining data.

@shinrich shinrich added this to the 8.0.0 milestone Jun 15, 2017
@shinrich shinrich self-assigned this Jun 15, 2017
SSL_set0_rbio(this->ssl, rbio);
free_handshake_buffers();
} else if (handShakeReader->is_read_avail_more_than(0)) {
this->handShakeReader->consume(this->handShakeBioStored);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this block same logic in other 2 places (line 447 and line 1078) in this class?
If so, it'd be great if we can avoid copy & past.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed.

@masaori335 masaori335 added this to 7.1.x backports in 7.x releases Jun 15, 2017
@masaori335 masaori335 added the Backport Marked for backport for an LTS patch release label Jun 15, 2017
@masaori335 masaori335 modified the milestones: 7.1.0, 8.0.0 Jun 15, 2017
@shinrich
Copy link
Member Author

Thought on this some more this morning. Perhaps all we should do is replace the ink_release_assert with a ink_assert and fail the connection in this case. I think if the assert is triggered, the client is behaving badly. Of course we can try to stumble on (this code addition). Perhaps that is ok too. But at a minimum the ink_release_assert should be removed/replaced.

… remaining data.

Tidy up the fix and reduce cut-n-paste.
@shinrich
Copy link
Member Author

Pushed an updated PR that gathers up some of the cut-n-paste rbio code. I'm testing this on one of our systems.

@shinrich shinrich removed the WIP label Jun 16, 2017
@bryancall
Copy link
Contributor

[approve ci autest]

@zwoop
Copy link
Contributor

zwoop commented Jun 16, 2017

Testing on Docs / CI right now, once we land this on master (once reviewed), I can test on prod.

@shinrich
Copy link
Member Author

It has been successfully running on my box today. Of course that box was not seeing the release_assert before.

} else {
Debug("ssl", "Want read from socket");
read.triggered = 0;
Copy link
Member

Choose a reason for hiding this comment

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

This forces a blind tunnel to wait for the socket to be come read ready?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is correct. When the epoll shows data ready to read it will set the trigger.

@SolidWallOfCode
Copy link
Member

Nice cleanup of all that cut and paste stuff.

@shinrich shinrich merged commit 2a112d0 into apache:master Jun 17, 2017
@zwoop zwoop removed this from 7.1.x backports in 7.x releases Jun 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backport Marked for backport for an LTS patch release TLS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants