[FIX] server, bus: filedescriptor out of range in select

[moylop260]

Use the most efficient Selector implementation available on the current platform

Odoo supports only SelectSelector but it is a little obsolete

python >= 3.4 supports a new high-level library Selectors:

• https://docs.python.org/es/3/library/selectors.html

It could to auto-choose the following ones:

• SelectSelector
• PollSelector
• EpollSelector
• DevpollSelector
• KqueueSelector

Using the DefaultSelector class the most efficient implementation available on the current platform will be use:

• https://docs.python.org/3/library/selectors.html#selectors.DefaultSelector

It helps to support better the resources of the system

Using SelectSelector you are not able to run workers >=255

If you set ulimit -n 10240 and run odoo-bin --workers=255
the following error is raised:

Traceback (most recent call last):
File "odoo/service/server.py", line 926, in run
    self.sleep()
File "odoo/service/server.py", line 852, in sleep
    sel.select(self.beat)
File "python3.8/lib/python3.8/selectors.py", line 323, in select
    r, w, _ = self._select(self._readers, self._writers, [], timeout)
ValueError: filedescriptor out of range in select()

But using PollSelector it is not reproduced even using more workers

Most of platform supports PollSelector but using DefaultSelector we can be sure
that even too old system are supported too

And using this High-level library will allow to use the future new improvements

e.g. Epoll has better performance improvements

More info about:

• https://devarea.com/linux-io-multiplexing-select-vs-poll-vs-epoll
• Update connection polling to use something other than select.select() redis/redis-py#486

Forcing the current SelectSelector:

• 

Using the DefaultSelector:

• 

Even if you don't need too many workers because of horizontal autoscaling it will have better performance and auto-evolution in the future python versions or systems

If you like this PR for v14.0 you can check the following PR:

• 14.0 - [FIX] server.py: Fix file descriptor error Vauxoo/odoo#477

[robodoo]

Pull request status dashboard.

[moylop260]

@odony

This is the change I mentioned you a few months ago in twitter

cc @xmo-odoo

[Julien00859]

Hello there,

Thank you for your contribution, indeed we shall investigate whether we can drop the old select(2) for a modern alternative. IIRC it will use epoll(7) on most Linux, kqueue on unix and iocp on windows, all much more capable and fast than select(2).

I'll take some time to fetch and test your branch locally later today. In the meantime, could you please change here and there the calls to select()? For now you are passing the timeout as a positional argument, I think it'll more readable if we pass it as a keyword argument e.g. sel.select(timeout=self.multi.beat), what do you think?

[Julien00859]

The ci/styles is complaining that you don't use the mask variable from your loop. The way to get rid of this is to prefix the mask variable with an understore: for ..., _mask in ...

[moylop260]

Hi @Julien00859

Thanks for you review

I just used timeout keyword in the method and fixed the lint

Let me know if we need to change something else

[Julien00859]

I did a quick benchmark on my machine. The values are not be be taken by themselves, the important bit is the difference between master and your branch. Empty clean databases, only the test_http module installed. Server running without crons and with 16 http workers (the maximum for my machine). Tested with ab from apache2-utils, sending 24000 requests to /test_http/greeting, 24 parallels requests at a time, with a session set.

~ $ set session_id (curl -v odoo.localhost/test_http/greeting 2>&1| grep session_id | cut -d '=' -f2 | cut -d ';' -f1); echo $session_id
d875b005768a01a6f5876ab533806d4a0e60a46c
~ $ ab -n 24000 -c 24 -C session_id=$session_id 127.0.0.1:8069/test_http/greeting-none

Master:

This is ApacheBench, Version 2.3 <$Revision: 1843412 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking 127.0.0.1 (be patient)


Server Software:        Werkzeug/0.16.1
Server Hostname:        127.0.0.1
Server Port:            8069

Document Path:          /test_http/greeting-none
Document Length:        9 bytes

Concurrency Level:      24
Time taken for tests:   19.844 seconds
Complete requests:      24000
Failed requests:        0
Total transferred:      7224000 bytes
HTML transferred:       216000 bytes
Requests per second:    1209.44 [#/sec] (mean)
Time per request:       19.844 [ms] (mean)
Time per request:       0.827 [ms] (mean, across all concurrent requests)
Transfer rate:          355.51 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.4      0      15
Processing:     3   19 113.1     12    2836
Waiting:        3   19 113.1     12    2835
Total:          3   19 113.2     12    2836

Percentage of the requests served within a certain time (ms)
  50%     12
  66%     14
  75%     15
  80%     15
  90%     18
  95%     21
  98%     28
  99%     51
 100%   2836 (longest request)

This branch:

This is ApacheBench, Version 2.3 <$Revision: 1843412 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking 127.0.0.1 (be patient)


Server Software:        Werkzeug/0.16.1
Server Hostname:        127.0.0.1
Server Port:            8069

Document Path:          /test_http/greeting-none
Document Length:        9 bytes

Concurrency Level:      24
Time taken for tests:   20.762 seconds
Complete requests:      24000
Failed requests:        0
Total transferred:      7224000 bytes
HTML transferred:       216000 bytes
Requests per second:    1155.97 [#/sec] (mean)
Time per request:       20.762 [ms] (mean)
Time per request:       0.865 [ms] (mean, across all concurrent requests)
Transfer rate:          339.79 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.8      0      30
Processing:     3   20 114.9     13    2706
Waiting:        3   19 114.8     12    2706
Total:          3   20 115.0     13    2706

Percentage of the requests served within a certain time (ms)
  50%     13
  66%     14
  75%     15
  80%     16
  90%     19
  95%     23
  98%     31
  99%     51
 100%   2706 (longest request)

The total time, 19s844 in master vs 20s762 in your branch bothers me. There is a 4.5% perf loose. Would you mind running a benchmark on your side too?

[moylop260]

@Julien00859

I think if you run the same test using even the same branch you will have different results in the Time taken for tests

Try running almost 3 times the same test and you will see it

[moylop260]

@Julien00859

I ran the same tests too many times and I saw the same results than you ~4.5% perf loose

I just realized that other kind of Selectors is using timeout sleep in ms instead of s

So, it needs a extra compute to convert

• math.ceil(timeout * 1e3)

and the selectors library is validating the input and parsing the output in order to be all them compatible with all Selectors using the same code

So the time for each request using Select directly (without high-level library) 0.467ms
vs EPoll with high-level library 0.447ms
Diff 0.020ms

Let me know if we should develop our own library using directly ms instead of seconds in order to avoid recomputing this value here:

• odoo/odoo/service/server.py

  Line 700 in 69e31db

  self.beat = 4

I mean,

self.beat = math.ceil(4 * 1e3) 

and add code to validate if the system is compatible with Poll if not using Select by default and using the original class directly without extra validations

Using this way the time could be similar using directly the original class but the code added will be even more (since that it could be considered as a custom faster high-level library)

but it will fix the filedescriptor error when you are using workers>=255

Let me know what should we do in order to change it, please

• Compatibility with current 5 selectors using high-level library ~4.5% perf loose
• Compatibility with only 2 selectors (Select & Poll) using directly the original classes with extra-validations without perf loose

[Julien00859]

Hello @moylop260 it is not up to me to decide, I merely did a quick code review and asserted the performances. Your last comment confuses me more than it helps, what new library? From the python documentation, the timeout must be specified in seconds, I don't understand why you say sometimes it takes ms.

https://docs.python.org/3/library/selectors.html#selectors.BaseSelector.select

If timeout > 0, this specifies the maximum wait time, in seconds.

The actual code is clean, I would be against we develop yet another abstraction layer around selector or that we reject kqueue/epoll/iocp. It is 4.5% slower, ok, it is what it is. Now we should wait and see what Olivier and Xavier have to say about this branch.

[moylop260]

I don't understand why you say sometimes it takes ms.

Yes, we are using a High-level library to avoid using each particular case of the selector in a low-level

Odoo is using Select in a low-level and it is using the timeout in seconds

But Poll in a low-level is using milliseconds

So, someone needs to transform the seconds used by the high-level library to miliseconds

It is in the following line:

• https://github.com/python/cpython/blob/c60414de7cefd092643ba200c2c045da1569c391/Lib/selectors.py#L410-L412

because low-level Poll is using milliseconds but the high-level library is standardizing for all Selectors in seconds

So, we can implement low-level Poll but using the timeout in milliseconds directly in the variables in order to avoid transforming on-the-fly, I mean, assign directly self.beat = math.ceil(4 * 1e3)

Another extra processing is transforming the output to match with all selectors in a standard way here:

• https://github.com/python/cpython/blob/c60414de7cefd092643ba200c2c045da1569c391/Lib/selectors.py#L418-L427

So, it is spending the 0.020ms

[moylop260]

@odony

What could we do to go forward with this?

[Julien00859]

The cause of the slight perf loose is maybe because in various places we re-create a select object and re-register many file descriptors within.

[Julien00859]

Here you create a new select() object and you re-register all the file descriptions every time we sleep. It would be better to create the select object once and to pro-actively add/remove file-description as they spawn and die.

[Julien00859]

Same as above.

[Julien00859]

Same as above.

[Julien00859]

Also, the only place where we select() for many (n) sockets is in the PreforkServer for WorkerHTTP, IMO we shouldn't attempt to change the selector of the others threads/workers

[Julien00859]

I couldn't push on your branch so I made this one #97831

[moylop260]

Superseded by #97831