WIP: Read Only Context

[daschl]

No description provided.

[daschl]

@idelpivnitskiy of curse this will not cover the case where someone grabs the context, then sends, and then writes into it. So we likely need a "SendAwareContextMap" and always return that one wrapped.

[idelpivnitskiy]

Yes, I think whenever this method is invoked it should hold a reference to this response and always run response.checkSent() for any modifications. Or it can just take a lambda that will be referenced by this::checkSent().

[daschl]

@idelpivnitskiy interesting part here are the failures where GRPC is using the response context for some trailer work:

java.lang.UnsupportedOperationException: This ContextMap is read-only
	at io.servicetalk.concurrent.internal.ReadOnlyContextMap.isReadOnly(ReadOnlyContextMap.java:126) ~[servicetalk-concurrent-internal/:?]
	at io.servicetalk.concurrent.internal.ReadOnlyContextMap.put(ReadOnlyContextMap.java:57) ~[servicetalk-concurrent-internal/:?]
	at io.servicetalk.grpc.netty.RequestResponseContextTest.setTrailersContext(RequestResponseContextTest.java:254) ~[servicetalk-grpc-netty/:?]
	at io.servicetalk.grpc.netty.RequestResponseContextTest.access$400(RequestResponseContextTest.java:82) ~[servicetalk-grpc-netty/:?]
	at io.servicetalk.grpc.netty.RequestResponseContextTest$BlockingTesterServiceImpl.testResponseStream(RequestResponseContextTest.java:372) ~[servicetalk-grpc-netty/:?]
	at io.servicetalk.grpc.netty.TesterProto$Tester$ServiceFactory$Builder$3.handle(TesterProto.java:1878) ~[servicetalk-grpc-netty/:?]

	at io.servicetalk.concurrent.internal.ReadOnlyContextMap.isReadOnly(ReadOnlyContextMap.java:126) ~[servicetalk-concurrent-internal/:?]
	at io.servicetalk.concurrent.internal.ReadOnlyContextMap.put(ReadOnlyContextMap.java:57) ~[servicetalk-concurrent-internal/:?]
	at io.servicetalk.grpc.netty.RequestResponseContextTest.setTrailersContext(RequestResponseContextTest.java:254) ~[servicetalk-grpc-netty/:?]
	at io.servicetalk.grpc.netty.RequestResponseContextTest.access$400(RequestResponseContextTest.java:82) ~[servicetalk-grpc-netty/:?]
	at io.servicetalk.grpc.netty.RequestResponseContextTest$BlockingTesterServiceImpl.testBiDiStream(RequestResponseContextTest.java:351) ~[servicetalk-grpc-netty/:?]
	at io.servicetalk.grpc.netty.TesterProto$Tester$ServiceFactory$Builder$1.handle(TesterProto.java:1772) ~[servicetalk-grpc-netty/:?]

[idelpivnitskiy]

Bcz initially it's used only in one place, consider starting with a pkg-private class under http-api package

[idelpivnitskiy]

After you move to http-api, consider rewording this message to smth that will help understand the problem better in HTTP context, explaining why it's read-only at that point.

[idelpivnitskiy]

Actually, we can just use response.checkSent() to have a consistent exception.

[idelpivnitskiy]

Yes, I think whenever this method is invoked it should hold a reference to this response and always run response.checkSent() for any modifications. Or it can just take a lambda that will be referenced by this::checkSent().

[idelpivnitskiy]

interesting part here are the failures where GRPC is using the response context for some trailer work

hm, I looked at gRPC RequestResponseContextTest. gRPC is a bit different. Context there is not part of the BlockingStreamingGrpcServerResponse, it's part of GrpcServiceContext. So, it's not obvious from the API that they are related to response.sendMetaData().

Then I checked how it's used in ContextHttpServiceFilter and it seems legit (thread safe) to me even if it's after response.sendMetaData(), because it happens before writer.close() and then it's read inside ContextHttpServiceFilter via response.transform operation, which guarantees that payloadComplete will be invoked after setTrailersContext (inside try-with-resources, like 372).

That makes me feel that maybe we should not restrict writability of the response context. It should be fine as long as it's sequenced correctly. But I'm not sure how to make it easier to understand in javadoc of the response context.

[daschl]

Ok, let's close this and reconsider a different approach w.r.t documenting it.