opened
kstreee
username_0
### What does this PR do?

By implementing MessageClientPool for AsyncReqMessageClient (ZMQ),SaltMessageClient (TCP),
avoiding blocking waiting of a thread.

### What issues does this PR fix or reference?

This is an additional commit of #37878,  and I resolved all items in a todo list of this issue, #38093.

### Previous Behavior

1. TCP version of MessageClient could be hanging while writing a data to a socket. ZMQ MessageClient was already patched by a former PR #37878, but TCP version of MessageClient haven't been patched (this issue #38093 mentioned about it).
2. Couldn't set `sock_pool_size` explicitly by config files, thus I had modified the pool size value in code to use ZMQ version of MessageClient pool.

### New Behavior

1. By wrapping SaltMessageClient, TCP version of message client can avoid blocking waiting, like a ZMQ case.
2. By making `sock_pool_size` as a config parameter, the size of socket pool could be set explicitly by users.

### Tests written?

Yes
Implements MessageClientPool to avoid blocking waiting for zeromq and tcp communications.
issue
created
jacksontj
username_1
The TCP transport shouldn't be blocking the thread on socket writes as it it using nonblocking sockets wrapped in tornado coroutines. ZMQ shouldn't either (should also be using nonblocking sockets and coroutines). Are we seeing blocking behavior? If so, is it easy to reproduce? If we are in fact seeing blocking behavior there is most likely a bug-- no need to work around it in code (since the libraries are supposed to do it already).
comment
created
cachedout
username_2
@DmitryKuzmenko Could you also swing by here and let us know what you think?
comment
created
kstreee
username_0
In my case, I experienced 'blocking waiting' (I think I miss-chose an expression, it is not blocking an entire thread, but blocking other queued jobs, detail descriptions will be following.) while processing Salt API requests with a ZMQ transport. Before saying the details of 'blocking behavior' examples with code, [the class documentary comment](https://github.com/saltstack/salt/blob/2016.11/salt/transport/zeromq.py#L848-L854) in `AsyncReqMessageClient` is referring this problem directly.

```
This class wraps the underylying zeromq REQ socket and gives a future-based
interface to sending and recieving messages. This works around the primary
limitation of serialized send/recv on the underlying socket by queueing the
message sends in this class. In the future if we decide to attempt to multiplex
we can manage a pool of REQ/REP sockets-- but for now we'll just do them in serial
```

Literally, the AsyncReqMessageClient is delivering async-like behaviors using `send_queue` for send/recv messages. By dequeuing message one by one from `send_queue` in AsyncReqMessageClient, the message client write a data to a ZMQ socket. [This block](https://github.com/saltstack/salt/blob/2016.11/salt/transport/zeromq.py#L957-L981) is the dequeuing block. And the 'blocking behavior' is caused by [this line](https://github.com/saltstack/salt/blob/2016.11/salt/transport/zeromq.py#L974) of code, `ret = yield future`. The `yield` code of dequeuing block makes other queued messages waits until the processing of a current message is finished.


Queued messages must wait until a former message is finished to be written. For example, a job which has plenty of host list (300+) as a target host, takes a long time to write the job data to a socket. Let's assume that Salt API faced this kind of situations. Then, other requests, which are queued after the problematic job, will wait until the problematic job data is finished to be written by the dequeuing block. Salt API can't response for while to the 'other requests' that is requested after the problematic job.


I thought that this problem could be resolved by several ways.
1. Re-design dequeuing block to dequeue simultaneously unlike current logic.
2. Wrap dequeuing block using 'pool' technique to avoid waiting while dequeuing.

The first solution is definitive and best way to solve this problem, but I think it is really risky and hard to fix this problem by re-designing and re-implementing. Because the entire code of AsyncReqMessageClient is highly wired to ZMQ behaviors.
The second solution is, I think, work around of this issue, however, with this solution, we don't have to modify the dequeuing logic in ZMQ and TCP transports. I think the second solution, which is this PR, is more efficient, and safer than the first solution.
(Obviously, we have a third option that implementing 'multiplexing', as mentioned in the class documentary comment.)

I apologize that I described this problem as 'blocking behaviors'. The expression is, definitely, too blurry and isn't accurate for this problem.
I hope that this comment will help to understand the problem what I faced in Salt API, and if there are any ambiguous points or if you have questions about this issue, It will be my pleasure to discuss through this PR.
comment
created
kstreee
username_0
In the case of a TCP transport, [this dequeuing block](https://github.com/saltstack/salt/blob/2016.11/salt/transport/tcp.py#L931-L950) cause the same problem that I described in the former comment.
comment
created
kstreee
username_0
By the way, while writing the description of this issue, suddenly raised some ideas and questions that is related to this issue.

1. It is available to embed 'pool' to each MessageClient for ZMQ and TCP. Instead of using single 'stream' ([`self.stream`](https://github.com/saltstack/salt/blob/2016.11/salt/transport/zeromq.py#L928) for ZMQ, [`self._stream`](https://github.com/saltstack/salt/blob/2016.11/salt/transport/tcp.py#L855) for TCP), by making 'stream' member variable as a list that contains multiple streams, the 'pool' can easily be embedded to each MessageClient class. And simply use the streams by using scheduling strategy, like the round-robin. I think this design is more seamless than current PR. This design is riskier but less tricky than my first solution.

2. I have assumed that using multiple connections of ZMQ (or TCP) is safe (For example, writing a data X and a data Y using connection A and connection B respectively). I'm sure the safeness of this kind of usage is responsible for ZMQ and TCP libraries but is it actually 'safe'? I am suddenly confused about the safety properties of the libraries 😂.. Practically saying, until now, there have been no problems of using that kind of ways. I set socket pool as 15 and have served plenty and high loads of requests, there weren't any problems that related to it. But, I'm gonna run some tests and find some references about it and share through additional comments. If you know any recommendable references, please share those for me.
comment
created
cachedout
username_2
@DmitryKuzmenko Could you please comment on this proposal?
comment
created
kstreee
username_0
@skizunov I wanted to make it sure that the pool closes sockets, but like as you said about it, the closing statements were redundant. I agreed what you commented. Thanks for your reviewing this.
I removed the redundant closing statements in an additional commit.
comment
created
cachedout
username_2
Go Go Jenkins!
comment
created
cachedout
username_2
Go Go Jenkins!
comment
created
cachedout
username_2
This appears to be leaking file handles. Tests are now failing with: `Too many open files (epoll.cpp:39)`
comment
created
kstreee
username_0
@username_2 I think the last modification is the cause of leaking file handles. I will fix and commit for the leaking with test cases soon. Thanks fou your inform :).
comment
created
kstreee
username_0
@username_2 I fixed the issue.
comment