Skip to content

Revert aa9478f06d613cd7b845e609a8c20c1ce116dad5#4581

Merged
junr03 merged 2 commits intomasterfrom
revert-aa9478f06d613cd7b845e609a8c20c1ce116dad5
Oct 2, 2018
Merged

Revert aa9478f06d613cd7b845e609a8c20c1ce116dad5#4581
junr03 merged 2 commits intomasterfrom
revert-aa9478f06d613cd7b845e609a8c20c1ce116dad5

Conversation

@junr03
Copy link
Copy Markdown
Member

@junr03 junr03 commented Oct 1, 2018

This PR reverts #4382. When deploying at Lyft we noticed crashes on here where we might be derefencing the connection_stats_ pointer after the point has been reset.

Note: this PR keeps the changes to the API made in the original PR but tags the field as not implemented. This is what we have done in the past for reverts that involve changes that change the API.

Jose Nino added 2 commits October 1, 2018 15:21
This reverts commit aa9478f.

Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Jose Nino <jnino@lyft.com>
@junr03
Copy link
Copy Markdown
Member Author

junr03 commented Oct 1, 2018

@AndresGuedez

@AndresGuedez
Copy link
Copy Markdown
Contributor

I will file an issue and take a look.

As @junr03 pointed out, it seems the delayed close timeout callback is being issued after a ConnectionImpl::closeSocket() call which resets the connection_stats_ pointer has occurred. This points to an interesting race since destruction of the ConnectionImpl should lead to disarming the timeout, so it's possible the onDelayedCloseTimeout() cb is triggering while the ConnectionImpl is in the deferred deletion stage.

@junr03 junr03 merged commit 9d32e5c into master Oct 2, 2018
jmarantz added a commit to jmarantz/envoy that referenced this pull request Oct 3, 2018
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@mattklein123 mattklein123 deleted the revert-aa9478f06d613cd7b845e609a8c20c1ce116dad5 branch October 4, 2018 21:54
aa-stripe pushed a commit to aa-stripe/envoy that referenced this pull request Oct 11, 2018
This PR reverts envoyproxy#4382. When deploying at Lyft we noticed crashes on here where we might be derefencing the connection_stats_ pointer after the point has been reset.

Note: this PR keeps the changes to the API made in the original PR but tags the field as not implemented. This is what we have done in the past for reverts that involve changes that change the API.

Signed-off-by: Jose Nino <jnino@lyft.com>
Signed-off-by: Aaltan Ahmad <aa@stripe.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants