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

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

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

moylop260
Copy link
Contributor

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:

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:

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:

@moylop260
Copy link
Contributor Author

@guewen

May I ask you for a review here, please?

Copy link
Member

@guewen guewen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

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

@moylop260
Copy link
Contributor Author

moylop260 commented Feb 16, 2022

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

@guewen
Copy link
Member

guewen commented Feb 17, 2022

Great!

@guewen guewen requested a review from sbidoul February 17, 2022 12:04
@moylop260 moylop260 force-pushed the 14.0-fix-filedesc-moy branch 3 times, most recently from 111810b to a1c49f6 Compare February 17, 2022 22:16
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
 - redis/redis-py#486
 - odoo/odoo#84684
Copy link
Member

@ovnicraft ovnicraft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions
Copy link

github-actions bot commented Aug 7, 2022

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 github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Aug 7, 2022
@guewen
Copy link
Member

guewen commented Aug 8, 2022

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

@guewen guewen removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Aug 8, 2022
@moylop260
Copy link
Contributor Author

@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:

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

@github-actions
Copy link

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 github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Dec 11, 2022
@github-actions github-actions bot closed this Jan 15, 2023
@gurneyalex gurneyalex added no stale Use this label to prevent the automated stale action from closing this PR/Issue. and removed stale PR/Issue without recent activity, it'll be soon closed automatically. labels Jun 27, 2023
@gurneyalex gurneyalex reopened this Jun 27, 2023
@OCA-git-bot
Copy link
Contributor

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

@gurneyalex
Copy link
Member

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

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
Copy link
Contributor

@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 OCA-git-bot merged commit d9542c9 into OCA:14.0 Jun 27, 2023
5 of 6 checks passed
@OCA-git-bot
Copy link
Contributor

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

Copy link
Member

@gurneyalex gurneyalex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved merged 🎉 no stale Use this label to prevent the automated stale action from closing this PR/Issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants