Ambiguity with COMPLETE flag in Response frame

[NiteshKant]

Background

Response Frame is designed to eliminate the need of exchanging an additional frame for COMPLETE in cases where it is known while emitting an onNext that there are no more items.

This optimization is pretty useful for Request-Response exchange when it is known beforehand that there would only be one response frame (post fragmented frame coalescing).

In practice, this is less useful for request-stream, request-channel interactions where the presence of a functional composition library makes it more complex to determine whether an item is the last in the stream.

Problem

Since there is no explicit indication that a RESPONSE frame is NEXT_COMPLETE or just COMPLETE, a way to determine whether one should emit an item before completing is to check whether this RESPONSE contains any data or not. Java Implementation uses this approach.

As shown in this PR, this approach is error-prone for request-response interaction as an empty payload converts to just emit onComplete(). I will consider this as an error as request-response should expect either a response or an error.

[tmontgomery]

I had a discussion about this from the Reactive Streams API perspective. Unless the implementation does batching, there isn't a means to signal an onNext that also is an onComplete. At least that I can figure out.

[benjchristensen]

this approach is error-prone for request-response interaction as an empty payload

Why would there be an empty payload? In request-response, I agree that either "onNext+onComplete" or "onError" is required.

A request_response should always receive a NEXT_COMPLETE. It should be an error to even send a response frame without a payload on request_response.

Do we need to add something to the spec that clarifies or enforces this?

[NiteshKant]

@benjchristensen I think empty content is a valid usecase, eg: HTTP 204 is a valid status code which does not have any content. One option could be that we also check for metadata length but that could also be empty especially if it is over H2 which does header compression.

@tmontgomery Yes that is my belief too. Even if we do batching, there may be a function downstream that adds something to the batch. eg: stream.window().concat(). So, I don't think there is an effective way of doing this apart from the degenerate case of the Publisher being a special instance of a Publisher that holds a realized value eg: Flowable.just() in RxJava.

[benjchristensen]

But this isn't HTTP, and onComplete doesn't carry messages, so something like an "HTTP 204" either would have to be delivered in an onNext payload, or as an onError message.

So no, I don't think it's valid, just because HTTP does it. ReactiveSocket isn't HTTP.

[NiteshKant]

would have to be delivered in an onNext payload, or as an onError message.

So are you suggesting that when the payload is empty, we have two options:

• Send two frames: one ON_NEXT and one COMPLETE frame.
• Send ERROR that the payload is empty.

(Currently, we send a NEXT_COMPLETE frame (RESPONSE with complete flag set))

If yes, then which one do you prefer?

[benjchristensen]

There should not be an empty request-response that just completes. That breaks the ReactiveStreams spec which does not allow nulls. So if someone tries to return a null (an empty response), then it has to be onError.

[NiteshKant]

I think empty byte buffer and null are two different cases.

[benjchristensen]

Then send an empty byte buffer, as long as your mimetype knows what to do with it.

[benjchristensen]

The point is that request/response expects a non-null response to represent success. The ReactiveSocket state machine will always process the payload and emit onNext for request/response.

[benjchristensen]

Sounds like we should nail this down in the spec ... basically for each of the interaction models, what the expected contract is.

• request/response
• request/stream
• channel
• fire-and-forget

[stevegury]

I think an empty (!= null) response is perfectly valid, thus we need to distinguish between an empty onNext and an onComplete.

I agree that we should specify all of that in the spec.

[benjchristensen]

How would "empty" work with something like the Single (or a 'Future') type which is expected to work with request/response? It expects an onSuccess call, and will blow up with onError if onNext+onComplete are not called.

Are you saying an empty response would only call onComplete and not invoke onNext?

[stevegury]

In the case of request/response implemented with Single there's no difference, if it is implemented with an Observable, I expect that an empty response trigger onNext then onComplete, right now, it only trigger onComplete.

It is more pathologic in the case of request/stream, where an empty response will be interpreted as an onComplete and then end the stream from the client pov.

[benjchristensen]

request/stream is fine, as an empty stream with onComplete is part of the spec. It's just request/response, where Future/Single as a type expects onSuccess/onFailure methods.

So I don't see how "there's no difference" with Single, since Single will blow up and cause an onError if an onComplete is received without an onNext. It expects a single onNext followed by an onComplete.

[abersnaze]

@NiteshKant are going for a Maybe like request/response?

[NiteshKant]

@abersnaze the current state is that request-response acts like a Maybe but the intent is to make it behave like a Single

[NiteshKant]

Reading through the exchange, it looks like sending an empty NEXT and a COMPLETE is preferable over sending an ERROR when a frame arrives with empty payload and complete flag set.
Unless, we do this as an exception for request-response, a side-effect of this is that every COMPLETE frame will send an empty NEXT.
I am not excited about making a special case only for request-response as currently response frame handling is independent of the interaction model.

Is this side-effect acceptable?

[benjchristensen]

Why are empty responses being sent? That's not what request/response is in ReactiveSocket.

As far as ReactiveSocket is concerned, a response payload has to be sent, or it's an error.

The application can choose to interpret a payload as "empty" based on mime-type, but why are we trying to make request/response not have a response?

[NiteshKant]

Ok, so the definition of empty response is perhaps not defined here. When I say:

it looks like sending an empty NEXT

I refer to empty as onNext with a zero-length buffer. I guess what you are referring to empty as just onComplete akin to Flowable.empty() in RxJava?

[stevegury]

I think the confusion is coming from the definition of "empty response".
Let's use an example, a service using UTF-8 as a mime-type, a returning the middle name of a user.

Returning "Jean-Philippe" or "" (without metadata) are two acceptable responses to me. In the second case, the client will only receive an onComplete without any call to onNext, I think it must receive a call to onNext before onComplete. (because for example, the client may have code that only run in the onNext).

[benjchristensen]

I guess what you are referring to empty as just onComplete akin to Flowable.empty() in RxJava?

@NiteshKant yes, an "empty" stream means no 'onNext', as per Flowable.empty(). And my argument has been that request/response can't be an empty stream without onNext.

@stevegury in the example you're giving, I agree with you, it should receive both 'onNext' and 'onComplete'.

If by "empty" it just means an empty string or byte array, then that's semantic to the application, but it is still a payload being sent via 'onNext', which is what is expected on a request/response.

[NiteshKant]

And my argument has been that request/response can't be an empty stream without onNext.

Agreed. I created this issue to move away from this current behavior.

If by "empty" it just means an empty string or byte array, then that's semantic to the application, but it is still a payload being sent via 'onNext', which is what is expected on a request/response.

Yes. The PR for the java implementation I linked to does this change to always treat a Response frame with Complete flag set to generate an onNext and then onComplete(). If there is no (meta)data ((meta)data length is 0) in the Response frame, then it sends an onNext with a zero length buffer.

Purpose of this issue

The above approach works BUT it has a side effect (as I mentioned here) that all Response frames with no data and Complete flag will always send an onNext() with a zero-length buffer.

The intent here is to see whether there is an opportunity to improve this state. Few options come to my mind:

• Send an onNext() with zero-length buffer ONLY if it is a request-response stream.
• Introduce a flag for NEXT_COMPLETE to indicate a zero-length payload with Complete.
• Do nothing and live with an onNext() with zero-length payload for a complete frame and no data/metadata. This is just an issue with request-stream and request-channel.

[stevegury]

Ideally, I would prefer to add a new flag indicating that it's a NEXT response.
Combining this with COMPLETE would indicate a NEXT_COMPLETE.

[benjchristensen]

How is having an extra flag any more clear that just looking at the data section being > 0 bytes?

Response frame: https://github.com/ReactiveSocket/reactivesocket/blob/master/Protocol.md#response-frame

     0                   1                   2                   3
     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |                           Stream ID                           |
    +---------------+-+-+-+-+-------+-------------------------------+
    |   Frame Type  |0|M|F|C| Flags |
    +-------------------------------+-------------------------------+
                         Metadata & Response Data

If C=1, and data == 0 bytes, then it is onComplete.
If C=0, and data >0 bytes, then it is onNext.
If C=1, and data >0 bytes, then it is onNext + onComplete.
If C=0, and data == 0 bytes, then it is a bad frame and should call onError or kill the entire connection.

Thus, pseudocode that ignores the 4th erroneous case would basically look like this:

if(data.size > 0) {
  onNext(data);
}
if(c==1) {
  onComplete();
}

Handling that 4th case, probably something like this:

if(data.size > 0) {
  onNext(data);
  if(c==1) {
    onComplete();
  }
} else {
 if(c==1) {
    onComplete();
  } else {
    onError("invalid frame with no data or completion");
  }
}

Considering this logic, which can all be done just by looking at the frame, I don't see a need for the extra flag. Is there an efficiency gain or some other behavior that can be achieved with the additional flag that can't be done by the reader using the above logic?

[benjchristensen]

By the way, I ran into this tonight and have this line in my code right now:

// temporary fix until https://github.com/ReactiveSocket/reactivesocket/issues/126 is resolved
.filter(p -> p.getData().remaining() > 0)

[NiteshKant]

If C=1, and data == 0 bytes, then it is onComplete.

Doesn't work for request-response as there won't be any onNext in this case.

[yschimke]

@benjchristensen great minds... rsocket/rsocket-java#214

[benjchristensen]

Doesn't work for request-response as there won't be any onNext in this case.

That would be an error in that case. The client library implementation still needs to maintain the contract of whatever interaction model it's within. So on request/response, C==1, data == 0 bytes is invalid and should cause onError.

Changing the specification with flags doesn't prevent the implementation needing to validate correct behavior.

[NiteshKant]

So on request/response, C==1, data == 0 bytes is invalid and should cause onError.

I think this is the main disagreement between us. @stevegury seems to agree that data == 0 bytes is valid. What do others think?

[yschimke]

I don't like the data == 0 bytes being a special value.

REQUEST_RESPONSE should be able to complete with a single empty payload.

REQUEST_STREAM should be able to contain empty payloads mid stream, at the end of stream and generally the sequence of payloads that a server implementor sends should be seen by the client in all cases by default. Just works

I think I disagree with the statement by @benjchristensen, "If C=0, and data == 0 bytes, then it is a bad frame and should call onError or kill the entire connection." In a stream, why isn't an empty payload acceptable. As a pure user of the reactivesocket protocol, why can't my stream of responses be time based e.g. every minute here is the data I received, possibly empty?

[benjchristensen]

why isn't an empty payload acceptable

Is 0 bytes a null? If so, that breaks the ReactiveStreams contract which doesn't support nulls.

If it's a semantically empty payload, such as a CBOR object that has no data, then that would not be 0 bytes. It would have some bytes to represent the CBOR object which then has no key/values in it.

[benjchristensen]

2.13 Calling onSubscribe, onNext, onError or onComplete MUST return normally except when any provided parameter is null in which case it MUST throw a java.lang.NullPointerException to the caller.

[benjchristensen]

If the type being communicate is byte[] however, a 0 byte payload does make sense.

[benjchristensen]

I have submitted a proposal to add the Next flag: https://github.com/ReactiveSocket/reactivesocket/pull/137/files

[yschimke]

Thanks, BTW this is a perfectly valid HTTP Response also. So happy we haven't special cased this out.

content-length: 0
content-type: application/octet-stream

[benjchristensen]

This was done in #137