Skip to content

Fixed init-order-fiasco for static slice table.#20052

Merged
arjunroy merged 1 commit intogrpc:masterfrom
arjunroy:writes_per_tcp_test_fix
Aug 28, 2019
Merged

Fixed init-order-fiasco for static slice table.#20052
arjunroy merged 1 commit intogrpc:masterfrom
arjunroy:writes_per_tcp_test_fix

Conversation

@arjunroy
Copy link
Copy Markdown
Contributor

@arjunroy arjunroy commented Aug 23, 2019 •
edited
Loading

Fixes init-order bug affecting #19819 which
was first exposed by this commit:
857375a

fix #19819

@arjunroy arjunroy added kind/bug priority/P1 release notes: no Indicates if PR should not be in release notes labels Aug 23, 2019
@arjunroy arjunroy self-assigned this Aug 23, 2019
@arjunroy arjunroy mentioned this pull request Aug 23, 2019
Closed
@arjunroy arjunroy requested a review from yashykt August 23, 2019 21:54
@arjunroy
Copy link
Copy Markdown
Contributor Author

arjunroy commented Aug 23, 2019

Specific change: since we cannot be certain of the ordering of global object initialization across compilation units, it is dangerous to define the static slice table globally. Instead, we use the static-function variable method which is guaranteed to initialize the table before the first access.

@AspirinSJL
Copy link
Copy Markdown
Contributor

AspirinSJL commented Aug 23, 2019

Does this fix #19819?

@yashykt
Copy link
Copy Markdown
Member

yashykt commented Aug 23, 2019

There seem to be a lot of failures

@arjunroy
Copy link
Copy Markdown
Contributor Author

arjunroy commented Aug 23, 2019

There seem to be a lot of failures

Fixed, that's because of stuff in test/ that I didn't convert over.

@hcaseyal
Copy link
Copy Markdown
Contributor

hcaseyal commented Aug 23, 2019

@AspirinSJL I haven't tested it on mac, but Linux has the exact same failure and I just confirmed that this fixes the issue on Linux. My guess is that it will fix the mac issue as well

@vjpai
Copy link
Copy Markdown
Contributor

vjpai commented Aug 23, 2019

I'll review when the contents of test/ are changed over as well.

@AspirinSJL
Copy link
Copy Markdown
Contributor

AspirinSJL commented Aug 23, 2019

@hcaseyal Thanks for confirming!

@arjunroy Then let's mark "fix #19819" in the PR description. Thanks!

@arjunroy
Copy link
Copy Markdown
Contributor Author

arjunroy commented Aug 26, 2019

Hmm, so actually this will break since this kind of static initialization appears to need libstdc++. Let me see if there are any alternatives...

@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla bot commented Aug 26, 2019 •
edited
Loading

CLA Check
The committers are authorized under a signed CLA.

@arjunroy arjunroy requested review from markdroth and soheilhy August 26, 2019 21:30
hcaseyal
hcaseyal approved these changes Aug 27, 2019
View reviewed changes
markdroth
markdroth reviewed Aug 27, 2019
View reviewed changes
markdroth
markdroth reviewed Aug 27, 2019
View reviewed changes
markdroth
markdroth reviewed Aug 27, 2019
View reviewed changes
@arjunroy
Fixed init-order-fiasco for static slice table.
2767acc 
Fixes init-order bug affecting grpc#19819 which
was first exposed by this commit:
grpc@857375a
@arjunroy arjunroy force-pushed the writes_per_tcp_test_fix branch from 7b98c04 to 2767acc Compare August 27, 2019 23:30
@arjunroy arjunroy merged commit 4162a3d into grpc:master Aug 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@markdroth markdroth markdroth approved these changes

@soheilhy soheilhy soheilhy approved these changes

@yashykt yashykt yashykt approved these changes

@apolcyn apolcyn Awaiting requested review from apolcyn

@AspirinSJL AspirinSJL Awaiting requested review from AspirinSJL

+2 more reviewers

@vjpai vjpai vjpai approved these changes

@hcaseyal hcaseyal hcaseyal approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@arjunroy arjunroy

Labels

kind/bug priority/P1 release notes: no Indicates if PR should not be in release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

C/C++ writes_per_rpc_test failed on MacOS

7 participants

@arjunroy @AspirinSJL @yashykt @hcaseyal @vjpai @soheilhy @markdroth