Skip to content

Split start from newCall, and specify method calling order restrictions. #1294

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

Closed
wants to merge 0 commits into from

Conversation

carl-mastrangelo
Copy link
Contributor

This is a fairly large change, with special care taken around synchronization. The main idea of this change is to make it possible to do secondary initialization on the stream before any network traffic happens.

}
headers.removeAll(GrpcUtil.AUTHORITY_KEY);
InProcessStream stream = new InProcessStream();
stream.serverStream.setListener(clientStreamListener);
Copy link
Member

Choose a reason for hiding this comment

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

If this is delayed, then serverTransportListener.streamCreated needs to be delayed, and so clientStream.setListener needs to be delayed, and that causes a race with shutdownStatus, such that it should be delayed. So basically everything should be delayed (it doesn't matter when the headers.removeAll is done, but might as well delay it with everything else).

I think that leaves in this method just new InProcessStream() and some lines to save method and headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, using the same style as Netty and OkHttp transports.

@carl-mastrangelo
Copy link
Contributor Author

@zhangkun83 Are the any more outstanding comments?

// Volatile to be readable without synchronization in the fast path.
// Writes are also done within synchronized(this).
private volatile ClientStream realStream;
// set to non null once both listener and unstartedStream are valid. After this point it is safe
Copy link
Contributor

Choose a reason for hiding this comment

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

s/unstartedStream/realStream

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@carl-mastrangelo carl-mastrangelo changed the title Split start from newCall Split start from newCall, and specify method calling order restrictions. Jan 6, 2016
@zhangkun83
Copy link
Contributor

LGTM

@carl-mastrangelo carl-mastrangelo deleted the fromPression4 branch January 6, 2016 20:16
@lock lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants