Skip to content

Rewrite the assert statement to avoid Socks Proxy triggering the assertions#5396

Merged
oknet merged 1 commit intoapache:masterfrom
oknet:i5396
May 5, 2019
Merged

Rewrite the assert statement to avoid Socks Proxy triggering the assertions#5396
oknet merged 1 commit intoapache:masterfrom
oknet:i5396

Conversation

@oknet
Copy link
Copy Markdown
Member

@oknet oknet commented Apr 30, 2019

No description provided.

@oknet oknet added the SOCKS label Apr 30, 2019
@oknet oknet added this to the 9.0.0 milestone Apr 30, 2019
@oknet oknet self-assigned this Apr 30, 2019
@oknet
Copy link
Copy Markdown
Member Author

oknet commented Apr 30, 2019

The assert statement is introduced by PR #4166.

Request @shinrich for review.

@oknet oknet requested a review from shinrich April 30, 2019 06:20
@oknet
Copy link
Copy Markdown
Member Author

oknet commented Apr 30, 2019

Since PR #4166 has been ported back to 7.1.x and 8.0.x, this PR is a candidate for them to fix the SocksProxy.

@zwoop @bryancall

@oknet oknet requested a review from SolidWallOfCode April 30, 2019 13:05
@zwoop
Copy link
Copy Markdown
Contributor

zwoop commented Apr 30, 2019

@ezelkow1 Lets put this on the radar for v7.1.7 as well.

@ezelkow1
Copy link
Copy Markdown
Member

👍 We've seen this crash in 8.1.x at least, so if the issue is on 7 it should definitely go in

@jrushford
Copy link
Copy Markdown
Contributor

@shinrich and @oknet we've see crashes here on 8.1. this looks like it will fix the crashes we've seen

@ezelkow1
Copy link
Copy Markdown
Member

ezelkow1 commented May 1, 2019

We pulled this in to a test 8.1.x build yesterday and were still seeing this assert hit

Copy link
Copy Markdown
Member

@shinrich shinrich left a comment

Choose a reason for hiding this comment

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

Seems reasonable. I had not considered the socks path.

@oknet oknet merged commit c934b15 into apache:master May 5, 2019
@oknet
Copy link
Copy Markdown
Member Author

oknet commented May 5, 2019

@ezelkow1 Have your filed an issue ? Please show me your assert and result of gdb backtrace full command.

@ezelkow1
Copy link
Copy Markdown
Member

ezelkow1 commented May 5, 2019

@oknet I have not filed an issue since I was just pulling this change in before it was merged. I cant really get a full backtrace, only what bits we have stored from BT since the only way to get this is to run it in production for a while and wait to hit it.

From BT I can see that pending_action is not null, so it must be failing on the second part of the statement, so the pending's cont is different from the vc's. I dont have any insight into the contents of 'vc' during the crashes either so I cant personally do a comparison of the continuation values

@zwoop
Copy link
Copy Markdown
Contributor

zwoop commented Mar 26, 2020

This has merge conflicts in 8.1.x

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.

5 participants