cowboy icon indicating copy to clipboard operation
cowboy copied to clipboard

cowboy_stream_h problems in grpc streaming

Open tony612 opened this issue 4 years ago • 4 comments

I got two problems when using cowboy_stream_h to implement grpc streaming calls:

  1. I tried to use auto mode, but got an error when calling cowboy_req:read_body(Req, [{length, auto}]). I believe this is because this line https://github.com/ninenines/cowboy/blob/13cf0baa41ad0b6e96f7fea0b95a8002e930e4a2/src/cowboy_req.erl#L520
  2. Streaming client may send only one empty body with fin to info indicate the streaming request is done. But current cowboy_stream_h can't handle this because https://github.com/ninenines/cowboy/blob/13cf0baa41ad0b6e96f7fea0b95a8002e930e4a2/src/cowboy_stream_h.erl#L161-L166 To solve this, we may need to send the body back when fin, like adding this as before the first read_body info:
info(StreamID, Info={read_body, Pid, Ref, auto, infinity}, State=#state{
		read_body_is_fin=fin, read_body_buffer=Buffer, body_length=BodyLen}) ->
	send_request_body(Pid, Ref, IsFin, BodyLen, Buffer),
         % We can avoid sending flow if Buffer is empty
	do_info(StreamID, Info, [{flow, byte_size(Buffer)}],
		State#state{read_body_buffer= <<>>});

tony612 avatar Dec 25 '19 06:12 tony612

You can't use read_body with auto at the moment (or ever, to be honest, it's not really the same thing) but you can send the message directly. Now there's a cowboy_req:cast(Req, Msg) to send that message.

It should special case fin indeed.

essen avatar Dec 25 '19 10:12 essen

I am thinking of reworking read_body auto into a sort of active,N where we would be able to send a message asking for the body to be sent as it comes in for a total of N messages, followed by a type of passive message like active,N has. Then it can be requested again. It could also be canceled if necessary.

I think we should also be able to request a minimum flow value when doing this. So perhaps a command like min_flow needs to be added. Then if we set min_flow and there's already a high enough flow value there's no need to send WINDOW_UPDATE or other and no risk to overflow.

I am thinking of making new messages rather than reuse read_body. Maybe {active_read_body, Pid, Ref, MinFlow, N} where N can be false to cancel. The request_body message can be kept as is, however an extra message {passive_read_body, Ref} needs to be sent once N reaches 0 or is otherwise canceled. Sending active_read_body twice in a row would result in the N values being added up and the MinFlow updated.

essen avatar Jan 06 '20 11:01 essen

@essen Then we need to store the data as a list of binary instead binary like what we do now, right? And when read_body is called, we should call iolist_to_binary first?

tony612 avatar Jan 07 '20 08:01 tony612

In active mode, you don't call read_body.

And either way there's never a need to use lists of binaries when you can just do binary appends. That's how read_body works.

essen avatar Jan 07 '20 08:01 essen