Skip to content

Validate send quota again after acquiring writable channel #1367

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jul 31, 2017
Merged

Validate send quota again after acquiring writable channel #1367

merged 4 commits into from
Jul 31, 2017

Conversation

MakMukhi
Copy link
Contributor

No description provided.

@MakMukhi MakMukhi requested a review from menghanl July 10, 2017 23:14
@MakMukhi MakMukhi mentioned this pull request Jul 10, 2017
Closed
menghanl
menghanl reviewed Jul 11, 2017
View reviewed changes
transport/http2_server.go Outdated
func (t *http2Server) applySettings(ss []http2.Setting) {
for _, s := range ss {
if s.ID == http2.SettingInitialWindowSize {
atomic.AddUint64(&t.outQuotaVersion, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we apply settings more than once?
I'm thinking if we could use store instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, InitialWindowSize settings frame can be sent multiple times during the life of a connection. In fact, that's why we're seeing this error. C decreases the IWS during it's bdp calculations causing the issue to occur.

transport/http2_server.go
// InitialWindowSize settings frame must have been received after we
// acquired send quota but before we got the writable channel.
// We must forsake this write.
t.sendQuotaPool.add(ps)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to also return the stream quota?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, to keep things consistent. When the settings frame is received it adds to the current quota. Since we took something out of it earlier, we must add that back since we're not going to send anything.

@dfawley dfawley assigned menghanl and MakMukhi and unassigned menghanl Jul 21, 2017
@MakMukhi MakMukhi force-pushed the invalidate_out_quota branch from 2bdf68a to 269d4a2 Compare July 26, 2017 18:50
@dfawley dfawley changed the title Validate send qouta again after acquring writable channel. Validate send quota again after acquiring writable channel Jul 28, 2017
dfawley
dfawley reviewed Jul 28, 2017
View reviewed changes
transport/http2_client.go

bdpEst *bdpEstimator
bdpEst *bdpEstimator
outQuotaVersion uint64
Copy link
Member

Choose a reason for hiding this comment

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

Move version into quota.

Copy link
Member

Choose a reason for hiding this comment

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

No, don't. There should only be one version per connection, not one per stream-quota.

@MakMukhi MakMukhi merged commit 7c7afa0 into grpc:master Jul 31, 2017
@brotherlogic
Copy link

brotherlogic commented Jul 31, 2017 •
edited by menghanl
Loading

Not sure, but this change appears to have broken compilation on ARM compilers. Running a simple binary in a raspberry pi results in this:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x4 pc=0x1265c]

goroutine 36 [running]:
sync/atomic.addUint64(0x1087c514, 0x1, 0x0, 0x0, 0x0)
	/usr/local/go/src/sync/atomic/64bit_arm.go:31 +0x4c
google.golang.org/grpc/transport.(*http2Server).applySettings(0x1087c460, 0x108737c0, 0x3, 0x4)
	/home/simon/code/src/google.golang.org/grpc/transport/http2_server.go:938 +0x130
google.golang.org/grpc/transport.(*http2Server).controller(0x1087c460)
	/home/simon/code/src/google.golang.org/grpc/transport/http2_server.go:1044 +0x1b8
created by google.golang.org/grpc/transport.newHTTP2Server
	/home/simon/code/src/google.golang.org/grpc/transport/http2_server.go:221 +0x860

@MakMukhi
Copy link
Contributor Author

MakMukhi commented Jul 31, 2017 •
edited
Loading

This is bizarre; t.outQuotaVersion should've been initialized with the transport struct. Does this help?

Wait, is it a 32bit system? If so, try this instead.

@brotherlogic
Copy link

I'm not sure it's strictly 32 bit - I found that this: https://patch-diff.githubusercontent.com/raw/grpc/grpc-go/pull/1410.patch fixes the problem. The change you suggested above did not work.

@MakMukhi
Copy link
Contributor Author

MakMukhi commented Jul 31, 2017 •
edited
Loading

The change that you're pointing to only moves the field to the beginning of the struct. Am I reading it right? Why does it work?

@brotherlogic
Copy link

I do not pretend I understand why - it's something related to golang/go#599; I robbed the fix from fsouza/go-dockerclient@7a6bd15

@MakMukhi
Copy link
Contributor Author

MakMukhi commented Aug 1, 2017

The issue that you're pointing too is about making 64-bit variables work on 32-bit machines. In that case the second fix that I provided should work.
Did you try both of them?

@brotherlogic
Copy link

Ah didn't see the second fix - that did the trick also.

@MakMukhi
Copy link
Contributor Author

MakMukhi commented Aug 1, 2017

Cool, I've created a pull request to do just that. We'd rather use a 32-bit number than going the alignment route.

@brotherlogic
Copy link

Awesome thanks!

@MakMukhi
Copy link
Contributor Author

MakMukhi commented Aug 1, 2017

Thanks for notifying.

@dfawley dfawley added this to the 1.6 Release milestone Aug 28, 2017
@dfawley dfawley added this to the 1.6 Release milestone Aug 28, 2017
@MakMukhi MakMukhi deleted the invalidate_out_quota branch May 4, 2018 02:05
@lock lock bot locked as resolved and limited conversation to collaborators Oct 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@dfawley dfawley dfawley approved these changes

+1 more reviewer

@menghanl menghanl menghanl approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@MakMukhi MakMukhi

Labels

Type: Bug

Projects

None yet

Milestone

1.6 Release

Development

Successfully merging this pull request may close these issues.

4 participants

@MakMukhi @brotherlogic @menghanl @dfawley