queue icon indicating copy to clipboard operation
queue copied to clipboard

[FIX] queue_job: runner - filedescriptor out of range in select

Open moylop260 opened this issue 3 years ago • 6 comments

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
  • https://github.com/redis/redis-py/issues/486
  • https://github.com/odoo/odoo/pull/84684

moylop260 avatar Feb 15 '22 22:02 moylop260

@guewen

May I ask you for a review here, please?

moylop260 avatar Feb 15 '22 22:02 moylop260

Hi @guewen

Thanks for your review

do you have a setup with more than 255 workers? I'm curious about numbers now! 🤩

Yes, we have a customer using monolith server and they need to support 5k concurrent website users purchasing BTW The queues were a important key to reach the focus Thank you for this awesome module @guewen

After running stress testing we reach the focus using 300 workers (But I have detected that using 255 is reproduced the error)

But it can be even reproduced it locally:

Forcing the current SelectSelector used by Odoo:

  • Screen Shot 2022-02-15 at 14 28 00

Using the DefaultSelector:

  • Screen Shot 2022-02-15 at 14 32 47

I know a better option is using horizontal autoscaling with small servers

But even using a DefaultSelector has performance improves and it has a better evolution over different system over the time

Thanks for asking

moylop260 avatar Feb 16 '22 14:02 moylop260

Great!

guewen avatar Feb 17 '22 12:02 guewen

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

github-actions[bot] avatar Aug 07 '22 12:08 github-actions[bot]

@moylop260 do you use this in production succesfully? Is it safe to merge?

guewen avatar Aug 08 '22 08:08 guewen

@guewen do you use this in production succesfully? Is it safe to merge?

We have already released in production and it is working well

However, if you are using workers>=255 Odoo itself will raise errors

So, don't have sense to merge this PR if Odoo is not fixing it first

The related PR is:

  • https://github.com/odoo/odoo/pull/84684

Let me ask if they agree with the change or if it needs something else

After merge it, I think we can use this way too here

Thanks for follow-up

moylop260 avatar Aug 08 '22 23:08 moylop260

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

github-actions[bot] avatar Dec 11 '22 12:12 github-actions[bot]

Hi @guewen, some modules you are maintaining are being modified, check this out!

OCA-git-bot avatar Jun 27 '23 12:06 OCA-git-bot

/ocabot merge minor

gurneyalex avatar Jun 27 '23 14:06 gurneyalex

Hey, thanks for contributing! Proceeding to merge this for you. Prepared branch 14.0-ocabot-merge-pr-408-by-gurneyalex-bump-minor, awaiting test results.

OCA-git-bot avatar Jun 27 '23 14:06 OCA-git-bot

@gurneyalex The merge process could not be finalized, because command git push origin 14.0-ocabot-merge-pr-408-by-gurneyalex-bump-minor:14.0 failed with output:

To https://github.com/OCA/queue
 ! [rejected]        14.0-ocabot-merge-pr-408-by-gurneyalex-bump-minor -> 14.0 (fetch first)
error: failed to push some refs to 'https://***@github.com/OCA/queue'
hint: Updates were rejected because the remote contains work that you do
hint: not have locally. This is usually caused by another repository pushing
hint: to the same ref. You may want to first integrate the remote changes
hint: (e.g., 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

OCA-git-bot avatar Jun 27 '23 14:06 OCA-git-bot

Congratulations, your PR was merged at 5e6d44f4acc887af5d70879abc1ed6aaed7a079a. Thanks a lot for contributing to OCA. ❤️

OCA-git-bot avatar Jun 27 '23 14:06 OCA-git-bot