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

[IMP][POC] server: check signaling in parent process #91253

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Xavier-Do
Copy link
Contributor

@Xavier-Do Xavier-Do commented May 12, 2022

When started with -d, registries are pre initialized before the workers
are spawned.
This means that the parent process will have some registries in memory
that will be available to the workers after the fork.
If the parent process never checks signaling and the sequence changes,
the copied registry will be reset at the first request.
This means that a new registry will be create after the fork everytime a
worker is recycled.
By checking the registry in the parent worker, we ensure that the forked
process will have a registry up to date (except if another change is
made during the fork...).

@Xavier-Do Xavier-Do requested a review from rco-odoo May 12, 2022 15:42
@robodoo
Copy link
Contributor

robodoo commented May 12, 2022

@Xavier-Do
Copy link
Contributor Author

cc @odony

@C3POdoo C3POdoo requested a review from a team May 12, 2022 15:58
@C3POdoo C3POdoo added the RD research & development, internal work label May 12, 2022
@Xavier-Do Xavier-Do force-pushed the master-server-registry-signaling-check-xdo branch 2 times, most recently from da1c12a to ae57ea9 Compare May 13, 2022 08:16
@Julien00859 Julien00859 removed the request for review from a team May 13, 2022 08:16
Copy link
Member

@rco-odoo rco-odoo left a comment

Choose a reason for hiding this comment

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

For the record, this completely makes sense to me.
I suggest to test it in real-world conditions before merging.

@odony
Copy link
Contributor

odony commented May 18, 2022

Makes the server die pretty quickly, after a few minutes max.

Judging by the semi-random errors, I'd say that we're sharing the db connections through multiple processes (after fork), therefore bypassing the ConnectionPool locks and corrupting the connection states.

Traceback (most recent call last):
  File "/code/odoo/service/server.py", line 929, in run
    self.check_registries()
  File "/code/odoo/service/server.py", line 833, in check_registries
    registry.check_signaling()
  File "/code/odoo/modules/registry.py", line 649, in check_signaling
    with closing(self.cursor()) as cr:
  File "/code/odoo/modules/registry.py", line 742, in cursor
    return self._db.cursor()
  File "/code/odoo/sql_db.py", line 760, in cursor
    return Cursor(self.__pool, self.dbname, self.dsn, serialized=serialized)
  File "/code/odoo/sql_db.py", line 300, in __init__
    self._cnx = pool.borrow(dsn)
  File "<decorator-gen-2>", line 2, in borrow
  File "/code/odoo/tools/func.py", line 69, in locked
    return func(inst, *args, **kwargs)
  File "/code/odoo/sql_db.py", line 682, in borrow
    cnx.reset()
psycopg2.errors.ActiveSqlTransaction: DISCARD ALL cannot run inside a transaction block
2022-05-18 13:56:13,131 3979454 WARNING db odoo.sql_db: Cursor not closed explicitly
Please enable sql debugging to trace the caller.
Exception ignored in: <function Cursor.__del__ at 0x7fa714ff9ee0>
Traceback (most recent call last):
  File "/code/odoo/sql_db.py", line 337, in __del__
    self._close(True)
  File "/code/odoo/sql_db.py", line 438, in _close
    if not self._obj:
  File "/code/odoo/sql_db.py", line 540, in __getattr__
    return getattr(self._obj, name)
  File "/code/odoo/sql_db.py", line 540, in __getattr__
    return getattr(self._obj, name)
  File "/code/odoo/sql_db.py", line 540, in __getattr__
    return getattr(self._obj, name)
  [Previous line repeated 989 more times]
RecursionError: maximum recursion depth exceeded
2022-05-18 14:13:14,289 3993576 ERROR db odoo.service.server: error with status PGRES_TUPLES_OK and no message from the libpq
Traceback (most recent call last):
  File "/code/odoo/service/server.py", line 929, in run
    self.check_registries()
  File "/code/odoo/service/server.py", line 833, in check_registries
    registry.check_signaling()  
  File "/code/odoo/modules/registry.py", line 650, in check_signaling
    cr.execute(""" SELECT base_registry_signaling.last_value,
  File "/code/odoo/sql_db.py", line 353, in execute
    res = self._obj.execute(query, params)
psycopg2.DatabaseError: error with status PGRES_TUPLES_OK and no message from the libpq
2022-05-18 14:13:14,292 3993576 INFO db odoo.service.server: Stopping forcefully

@Xavier-Do Xavier-Do force-pushed the master-server-registry-signaling-check-xdo branch from ae57ea9 to 1eaf3a3 Compare May 18, 2022 15:10
When started with -d, registries are pre initialized before the workers
are spawned.
This means that the parent process will have some registries in memory
that will be available to the workers after the fork.
If the parent process never checks signaling and the sequence changes,
the copied registry will be reset at the first request.
This means that a new registry will be create after the fork everytime a
worker is recycled.
By checking the registry in the parent worker, we ensure that the forked
process will have a registry up to date (except if another change is
made during the fork...).
@Xavier-Do Xavier-Do force-pushed the master-server-registry-signaling-check-xdo branch from 1eaf3a3 to ca1b6d2 Compare May 19, 2022 09:52
@Julien00859
Copy link
Member

@Julien00859

@Julien00859
Copy link
Member

@odony not so long ago you told me that we do not recycle http workers after x requests, can you confirm it?

@Xavier-Do
Copy link
Contributor Author

Xavier-Do commented Mar 8, 2023

Note that with https://github.com/odoo/odoo/compare/ae57ea9c54bc634543e554b6a5b3f965b52f9a49..ca1b6d2600ca4a3cb2833523d602a066035fb6ab the previous issue should be solved if it is still useful

@odony
Copy link
Contributor

odony commented Mar 9, 2023

@odony not so long ago you told me that we do not recycle http workers after x requests, can you confirm it?

yes, we basically set the request limit very high in order to avoid recycling workers too often - if we don't have any reason to do so, it's just wasteful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RD research & development, internal work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants