Skip to content
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

Add multipart message support for REQ sockets #53

Merged
merged 4 commits into from
Jun 12, 2023
Merged

Add multipart message support for REQ sockets #53

merged 4 commits into from
Jun 12, 2023

Conversation

siiky
Copy link
Contributor

@siiky siiky commented Jun 12, 2023

Fix #52

@siiky
Copy link
Contributor Author

siiky commented Jun 12, 2023

I added the missing case to recv_multipart (I think) but when I tried it I still got a single binary. Then I noticed this lists_to_binary call:

chumak/src/chumak_req.erl

Lines 138 to 154 in f89a873

peer_recv_message(#chumak_req{state=wait_more_msg, last_peer_sent=From}=State, Message, From) ->
#chumak_req{msg_buf=Buffer, pending_recv=PendingRecv} = State,
NewBuffer = Buffer ++ [chumak_protocol:message_data(Message)],
case {chumak_protocol:message_has_more(Message), PendingRecv} of
%% if need to accumulate more message
{true, _} ->
{noreply, State#chumak_req{state=wait_more_msg, msg_buf=NewBuffer}};
%% the last message was received, but the client not called recv yet
{false, nil} ->
{noreply, State#chumak_req{state=wait_recv, msg_buf=NewBuffer}};
%% client already called recv
{false, PendingRecv} ->
FullMsg = binary:list_to_bin(NewBuffer),
gen_server:reply(PendingRecv, {ok, FullMsg}),

It looks like it's assumed only recv would be used. Is there an easy/immediate way to distinguish between a client having called recv or recv_multipart? I was thinking that if there isn't, the easiest thing would probably be to move the list_to_binary to the client process (i.e. chumak:recv()), and the socket process always sends the list of binaries.

@siiky
Copy link
Contributor Author

siiky commented Jun 12, 2023

Alright, I think this is functioning now and will serve me well on the project.

I don't like the is_multipart flag, feels like a duct tape hack. Waiting for your review before I squash the commits to merge.

@drozzy drozzy marked this pull request as ready for review June 12, 2023 16:08
@drozzy drozzy merged commit 3c50798 into zeromq:master Jun 12, 2023
@drozzy
Copy link
Member

drozzy commented Jun 12, 2023

Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multipart send/recv support for REQ
2 participants