rabbitmq / rabbitmq-common

Common library used by rabbitmq-server and rabbitmq-erlang-client
Other
66 stars 112 forks source link

Support 'passive' tag in 'recv' #398

Closed tomyouyou closed 4 years ago

tomyouyou commented 4 years ago

At present, 'rabbit_reader' receive data one by one. If setopts(Sock, [{active, Number}]), it can receive batch data at a time.

michaelklishin commented 4 years ago

Fair enough. Can you please describe a use case for this or provide any other context around what you are doing and why?

lukebakken commented 4 years ago

Thanks for taking the time to use RabbitMQ and improve it.

Having said that, the changes in this pull request have no relation to the pull request's description. Handling the *_passive messages has nothing to do with how much data a socket receives. In fact, changing the way active is currently used would certainly introduce bugs.

The tcp_passive and ssl_passive messages are already handled by the Other clause here:

https://github.com/rabbitmq/rabbitmq-common/blob/master/src/rabbit_net.erl#L135

This message gets handled by reader / writer processes in code like this:

https://github.com/rabbitmq/rabbitmq-server/blob/master/src/rabbit_reader.erl#L655-L658

This means that a *_passive message will result in a crash. Howewer, I have never seen a TCP/TLS socket transition to passive mode, resulting in a crash.

tomyouyou commented 4 years ago

@michaelklishin @lukebakken If only 'rabbit_net' are modified, readers will crash. I should put the readers's changes together in this PR. Like the following:

case Recv of
    {data, Data} ->
        recvloop(Deb, [Data | Buf], BufLen + size(Data),
                 State#v1{pending_recv = false});
    passive when CS =:= blocked -> 
        %% stay in passive mode
        mainloop(Deb, Buf, BufLen, State);
    passive ->
        case rabbit_net:setopts(Sock, [{active, CfgActiveN}]) of
            ok -> mainloop(Deb, Buf, BufLen, State);
            {error, Reason} -> stop(Reason, State)
        end;

In addition, Let me make a little suggestion about ‘Automatically delete unrecoverable WAL segment files #190’. For engineering applications, broker should not hang up at any time and should be able to heal itself.

michaelklishin commented 4 years ago

@tomyouyou it would be much easier to reason about your contributions if you provided an explanation as to why you want these changes to be accepted. What are you doing? Why? Why do you want to use sockets in RabbitMQ this way?

What you are doing right now is called a "patch bomb" in the open source community. A non-trivial patch provided without any context or details is not only really hard to review, it is also very hard to evaluate how much of a ticking time bomb it may be.

tomyouyou commented 4 years ago

@michaelklishin Yes, you are right. I should describe clearly the background or reason of PR. The purpose of this PR is to improve the efficiency of recv operation.

lukebakken commented 4 years ago

@tomyouyou there is no need to handle the tcp_passive or ssl_passive case, because it will never be encountered. Notice that active, once is the only socket option used: rabbitmq-socket-options.txt.

tcp_passive / ssl_passive will only be sent if the socket is in {active, N} mode (docs).

tomyouyou commented 4 years ago

@lukebakken I didn't copy all the changes of readers, which should include: 1) Discard 'rabbit_net:setopts(Sock, [{active, once}])' 2)On initialization, call 'rabbit_net:setopts(Sock, [{active, N}])' 3)Enter passive when blocked by call 'rabbit_net:setopts(Sock, [{active, -32768}])' 4)Enter active when unblocked by call 'rabbit_net:setopts(Sock, [{active, N}])' 5)add the processing of passive for recv

@michaelklishin All my PRs have been tested on a large scale by our project. I will not submit PR without any test. It's just that there's something wrong with the way I submit PR.

lukebakken commented 4 years ago

@tomyouyou we can't accept or review partial pull requests. The typical workflow is to open all pull requests in all repositories that have changes, and use the same name for the branch in each repository.

Since you have tested your changes, can you please provide information about why you made these changes? @michaelklishin requested this earlier -

https://github.com/rabbitmq/rabbitmq-common/pull/398#issuecomment-643578617

tomyouyou commented 4 years ago

@lukebakken My mistake, I should not submit partial changes. I didn't know how to submit a PR covering multiple repositories. Thank you for your guidance.