#grpc

hi, node keeps crashing when running the grpc tutorial with no rpc server running? is it normal?

it's not really crashing in fact just exiting without any message :(

an idea anyone?

for example, this executes only once:

I'm on Win7 with Node 7.5.0.

reinstalling everything just in case.

Still exiting on first call with "Error: Connect Failed" message.

Ylbam: if no server is running, "connect failed" seems very reasonable. What were you hoping would happen?

that my node app keeps running

@ejona: my app exit without any stack trace or anything

does grpc have some binary dependencies that could be failing?

You're seeing a stack trace?

nope nothing

it just exit after first execution

Earlier you said "exiting without any message" but then you said you see "Error: Connect Failed"

Just displaying the "Result:" log once

yep it runs the callback once

when using node-debug it says "program terminated" once we leave the callback

have tried it with node 6.9.5 and 7.5.0, same behavior

tried trapping 'uncaughtException' or 'exit' without success

it just closes the app

Hmmm... Node people aren't quite in this morning yet.

if you want to give it a try the test.proto corresponds to this file: https://github.com/grpc/grpc/blob/v1.0.0/exampl...

I don't have time to setup a node environment at the moment.

ok

will give it a try on Linux, might be a Windows specific bug

not crashing on linux

Ylbam: One of our node devs didn't know why you'd see that behavior.

It crashes on win7, will try on win 10.

@ejona Do you have a sec to talk about the plan for HTTP2 pseudo headers?

rmichela: Sorry, just saw your message. Do you mean what we'll do with :status?

i'm particularly interested in :authority as it pertains to the HandlerRegistry API

also all pseudo headers in general

Ah, grpc/grpc-java#2704

yes

We don't want to expose pseudo headers in Metadata. But Authority became a first-class thing.

:path is already a first-class thing.

So that only really leaves :status.

We'd like to remove it, but it isn't clear how to expose it. So we're just in a holding pattern with it.

We=me

my understanding of why you don't want to expose all pseudo headers directly is that doing so would tie uses of gRPC to the specifics of http/2

I haven't looked at your PR deeply, but I saw the title and was happy to see it.

Yeah. We also have an API issue where we don't want Metadata Keys to have colons in them.

If you read the JavaDocs, it says that you can't use colons. But the implementation lets you, some of the time.

i should look at how :path is being exposed above the transport layer

Whoa. We weren't using the lookupMethod with authority. Huh. Yeah. That's an oversight.

yeah. it was never implemented

path == methodName (modulo leading /)

the netty transport does a great job of sealing away all the pseudo headers from the higher layers

Okay. Then your change is probably best served by modifying streamCreated to also include authority as another argument.

(as I'm looking at the code)

(Even though the streamCreated is in an interface, it's an internal interface. We are okay with breaking it)

using the stream's Attributes was a bit of a stretch, but i didn't want to modify any existing interfaces

Yeah. I think you did something quite reasonable without knowing more about how we do things. It isn't very invasive.

But I'm okay with the invasive change.

Oh.

what do you think of Carl's comment "This is going to add extra allocation and AsciiString conversion in the serving path, where there was none before. Almost no users need this data. This would slow down all servers for the sake of proxies."

There's another option.

ServerStream could eventually get an authority() method. Maybe do that now.

I think that concern can be addressed a couple ways.

1) caching the last authority String and reusing it. It'll almost always be the same

2) future HPACK improvements where we cache results in the HPACK table.

I'd honestly be fine with doing neither of those today, but we are focusing enough of performance at this point that it can make sense to avoid some of the cost.

#2 is a bit much for now. But Carl could maybe be okay with saying we'll fix it when the HPACK caching is available.

there could also be an optional flag on NettyServerBuilder that turns this functionality on

in the transparent proxy use case i'm experimenting with, the authority can change with every request

I think we'd be fine with the performance hit if it was changing every time, because that is a rarer case and you are probably fine with the minor performance difference.

So in the "I don't want to argue with people" sense, I'd probably just do #1 for when the authority doesn't change.

That should be really easy. Like 2-3 lines.

If it turns out to be hard, then we could re-evaluate. But if it is easy, that's what I would suggest.

cache the authority string in the netty transport?

Yeah.

Err...

In the NettyServerHandler

Err... looking...

perhaps i'm missing something, but how would i know if the authority string has changed without incurring the cost of decoding the authority string?

The authority string is already decoded, but it is in the form of an AsciiString. You can simply compare the old AsciiString to the new AsciiString.

Instead of adding a new argument to streamCreated(), I'd suggest adding a ServerStream.authority() method. And then call that method from ServerImpl.

You can make the change to Netty first and we can do some initial review. But you'd also need to add the authority() method to in-process transport.

Probably not too hard in terms of lines of code, but in-process may be scary initially.

is there a better point for me to branch off of than the commit that starts 1.2.0?

Anywhere on master is good.

That branch point was pretty recent, so it's fine.

any place with stable unit tests?

i'm seeing a few failed tests when i build the latest master

Really? Which ones?

Huh. There is more flakiness or something.

integration netty tests

like io.grpc.testing.integration.Http2NettyLocalChannelTest > unimplementedMethod FAILED

newStream_duringShutdown?

unimplementedMethod...

Weird.

io.grpc.testing.integration.CascadingTest > testCascadingCancellationViaLeafFailure FAILED java.lang.NullPointerException: serviceDef

i grabbed the latest master and ran ./gradlew build

Neither of those have been flaky recently...

i have six NoSuchMethodErrors in io.grpc.testing.integration tests

That sounds fishy.

Try a gradle clean?

that worked

Yay. Not great that clean was necessary, but glad the commit seems to working a bit better now.

what does authority even mean when dealing with an inproc ServerStream?

just return the in process transport name?

So you could do null for now, honestly.

I could see having some sort of authority being useful for testing.

But because authority is part of security mechanisms it can make sense to be conservative.

Actually, the client probably already has a authority it expects. We'll probably plumb it directly from what the client "sent"

rmichela: you could pass what was provided to setAuthority(). You can see my comment there asking what we should do with it.

That would also allow you to test a proxy with the inprocess transport. Which is nice.

Hmm...

rmichela: That's not enough by itself. That would only do something if the authority was set in the call options. You would also use:

So that would mean passing the authority to the "new InProcessTransport"

which is preferred when authority is absent, null, or empty string?

Eh. Probably doesn't matter much at this point. I'd say null.

"less ressources" ? :)

Will more obviously break someone that uses it :)

|Pixel|: It requires 4 characters instead of two :-)

I meant run-time ressources ? :D

empty string is less likely to break consumers expecting a string, and Metadata has Metadata.EMPTY instead of null

what language level is gRPC? java 1.8?

sourceCompatibility = 1.6 :(

@ejona do you have a google-preferred, 1.6 compatible alternative to ConcurrentHashMap that supports computeIfAbsent()?

i'm trying to keep a map of AsciiString->String to avoid converting authority AsciiStrings that have already been seen by the NettyServerHandler

rmichela: the Netty handlers are thread-unsafe. They only execute from one thread at a time. Also, I think everyone would be very fine if it only cached the last authority.

The equivalent is to use ConcurrentHashMap.get() before putIfAbsent to avoid calculating unnecessarily. You may do slightly more computation, but it would still perform reasonably.