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

Return header in Stream.Header() if available #1281

Merged
merged 2 commits into from Jun 13, 2017

Conversation

Projects
None yet
4 participants
@ilonajulczuk
Contributor

ilonajulczuk commented Jun 6, 2017

Add additional nonblocking select clause that would
check if the header is available and return it if it is.

Current implementation of transport.go Stream Header often
doesn't return header, but a cancel error even though
the top level context is not canceled and the rpc
is successful and a header is actually available.
It's caused by the fact that select can choose any
"ready" channel.
Retrieving headers is flaky. With this fix, headers
are consistently retrieved for successful rpcs.

Return header in Stream.Header() if available
Add additional nonblocking select clause that would
check if the header is available and return it if it is.

Current implementation of transport Header often
doesn't return header, but a cancel error even though
the top level context is not canceled and the rpc
is successful and a header is actually available.
It's caused by the fact that select can choose any
"ready" channel.
Retrieving headers is flaky. With this fix, headers
are consistently retrieved for successful rpcs.
@ilonajulczuk

This comment has been minimized.

Show comment
Hide comment
@ilonajulczuk

ilonajulczuk Jun 6, 2017

Contributor

Hi, could someone help me debugging the tests failure?
Travis doesn't provide any helpful message:
travis Also it only fails for the go 1.6.

When I test the code myself with "make" the tests pass and the command returns 0 status code.

Contributor

ilonajulczuk commented Jun 6, 2017

Hi, could someone help me debugging the tests failure?
Travis doesn't provide any helpful message:
travis Also it only fails for the go 1.6.

When I test the code myself with "make" the tests pass and the command returns 0 status code.

@MakMukhi

This comment has been minimized.

Show comment
Hide comment
@MakMukhi

MakMukhi Jun 6, 2017

Contributor

Finding the failed test on travis is tough in general. I usually search for "FAIL:". In your case this test failed:
FAIL: TestKeepaliveServerEnforcementWithAbusiveClientWithRPC (2.00s)
transport_test.go:694: Test failed: Expected a GoAway from server.

Contributor

MakMukhi commented Jun 6, 2017

Finding the failed test on travis is tough in general. I usually search for "FAIL:". In your case this test failed:
FAIL: TestKeepaliveServerEnforcementWithAbusiveClientWithRPC (2.00s)
transport_test.go:694: Test failed: Expected a GoAway from server.

@menghanl

This comment has been minimized.

Show comment
Hide comment
@menghanl

menghanl Jun 6, 2017

Contributor

It seems the test is flaky.
I restarted the test, and it passed.
Filed issue #1283 to track the flaky test.

Contributor

menghanl commented Jun 6, 2017

It seems the test is flaky.
I restarted the test, and it passed.
Filed issue #1283 to track the flaky test.

@dfawley

This comment has been minimized.

Show comment
Hide comment
@dfawley

dfawley Jun 9, 2017

Contributor

This doesn't deal with the goAway error. How about this version?

var err error
select {
case <-s.ctx.Done():
	err = ContextErr(s.ctx.Err())
case <-s.goAway:
	err = ErrStreamDrain
case <-s.headerChan:
	return s.header.Copy(), nil
}
// Even if the stream is closed, header is returned if available.
select {
case <-s.headerChan:
	return s.header.Copy(), nil
default:
}
return nil, err
Contributor

dfawley commented Jun 9, 2017

This doesn't deal with the goAway error. How about this version?

var err error
select {
case <-s.ctx.Done():
	err = ContextErr(s.ctx.Err())
case <-s.goAway:
	err = ErrStreamDrain
case <-s.headerChan:
	return s.header.Copy(), nil
}
// Even if the stream is closed, header is returned if available.
select {
case <-s.headerChan:
	return s.header.Copy(), nil
default:
}
return nil, err
@menghanl

Thanks for the change. LGTM.

@menghanl menghanl merged commit 89caed9 into grpc:master Jun 13, 2017

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment