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

Forking process with edgedb connection is dangerous #144

Open
tailhook opened this issue Dec 29, 2020 · 4 comments
Open

Forking process with edgedb connection is dangerous #144

tailhook opened this issue Dec 29, 2020 · 4 comments
Assignees
Labels
bug Something isn't working high priority

Comments

@tailhook
Copy link
Contributor

In particular, if connection is used in both forks, it looks like this:

    Error on request:
    Traceback (most recent call last):
      File "/usr/local/lib/python3.6/dist-packages/werkzeug/serving.py", line 323, in run_wsgi
        execute(self.server.app)
      File "/usr/local/lib/python3.6/dist-packages/werkzeug/serving.py", line 312, in execute
        application_iter = app(environ, start_response)
      File "/usr/local/lib/python3.6/dist-packages/werkzeug/wrappers/base_request.py", line 238, in application
        resp = f(*args[:-2] + (request,))
      File "/work/serve_py_sync/main.py", line 48, in application
        return ROUTES[endpoint](request, **values)
      File "/work/serve_py_sync/main.py", line 39, in increment
        """, name=name)
      File "/usr/local/lib/python3.6/dist-packages/edgedb/blocking_con.py", line 117, in query_one
        io_format=protocol.IoFormat.BINARY,
      File "edgedb/protocol/blocking_proto.pyx", line 91, in edgedb.protocol.blocking_proto.BlockingIOProtocol.sync_execute_anonymous

      File "edgedb/protocol/blocking_proto.pyx", line 77, in edgedb.protocol.blocking_proto.BlockingIOProtocol._iter_coroutine

      File "edgedb/protocol/protocol.pyx", line 588, in execute_anonymous

      File "edgedb/protocol/protocol.pyx", line 269, in _parse

      File "edgedb/protocol/protocol.pyx", line 1057, in edgedb.protocol.protocol.SansIOProtocol.fallthrough

    edgedb.errors.ProtocolError: unexpected message type 'T'Error on request:
    Traceback (most recent call last):
      File "/usr/local/lib/python3.6/dist-packages/werkzeug/serving.py", line 323, in run_wsgi
        execute(self.server.app)
      File "/usr/local/lib/python3.6/dist-packages/werkzeug/serving.py", line 312, in execute
        application_iter = app(environ, start_response)
      File "/usr/local/lib/python3.6/dist-packages/werkzeug/wrappers/base_request.py", line 238, in application
        resp = f(*args[:-2] + (request,))
      File "/work/serve_py_sync/main.py", line 48, in application
        return ROUTES[endpoint](request, **values)
      File "/work/serve_py_sync/main.py", line 39, in increment
        """, name=name)
      File "/usr/local/lib/python3.6/dist-packages/edgedb/blocking_con.py", line 117, in query_one
        io_format=protocol.IoFormat.BINARY,
      File "edgedb/protocol/blocking_proto.pyx", line 91, in edgedb.protocol.blocking_proto.BlockingIOProtocol.sync_execute_anonymous

      File "edgedb/protocol/blocking_proto.pyx", line 77, in edgedb.protocol.blocking_proto.BlockingIOProtocol._iter_coroutine

      File "edgedb/protocol/protocol.pyx", line 588, in execute_anonymous

      File "edgedb/protocol/protocol.pyx", line 310, in _parse

      File "edgedb/protocol/protocol.pyx", line 1057, in edgedb.protocol.protocol.SansIOProtocol.fallthrough

    edgedb.errors.ProtocolError: unexpected message type '1'

So the issue is similar to #130.

We may use os.register_at_fork to fix this in python3.7.

@tailhook tailhook self-assigned this Dec 29, 2020
@tailhook tailhook added the bug Something isn't working label Dec 29, 2020
@elprans
Copy link
Member

elprans commented Dec 29, 2020

How do other database drivers handle this?

@1st1
Copy link
Member

1st1 commented Jan 5, 2021

How do other database drivers handle this?

Yeah, good question.

@fantix
Copy link
Member

fantix commented May 25, 2023

psycopg2/libpq simply doesn't support forking, but they are connection-based, not pool-based like the EdgeDB client. I looked into a few other drivers and seems like "fork" is a very rare keyword as in multiprocessing (very ocassionally someone complains postgres-rust doesn't work in Celery subprocesses) - I'd say they don't support forking.

Theoretically we could shutdown all pooled connections in a subprocess after forking, but there're also very marginal cases to take care of like forking happened during a query or transaction.

@tailhook
Copy link
Contributor Author

there're also very marginal cases to take care of like forking happened during a query or transaction.

The error above and very apparent security issue (i.e. wrong reply matched to a request) happens exactly in this case. So we have to choose which process these connections can continue in. Which is I think is the parent process. In other ones it's okay to return an error.

I'm not sure whether we want to throw a specific error or just some "ConnectionClosedError" in this case. Because I think this is very Python-specific issue. I think most other languages do not support full-featured fork (C/C++ being rare exception, but we don't have C bindings yet).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working high priority
Projects
None yet
Development

No branches or pull requests

4 participants