New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 697254 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

autoserv: stop using atfork + possibly delete paramiko host code

Project Member Reported by akes...@chromium.org, Mar 1 2017

Issue description

autoserv has this whacky incanation near the top of it:

try:
    import atfork
    atfork.monkeypatch_os_fork_functions()
    import atfork.stdlib_fixer
    # Fix the Python standard library for threading+fork safety with its
    # internal locks.  http://code.google.com/p/python-atfork/
    import warnings
    warnings.filterwarnings('ignore', 'logging module already imported')
    atfork.stdlib_fixer.fix_logging_module()
except ImportError, e:
    from autotest_lib.client.common_lib import global_config
    if _CONFIG.get_config_value(
            'AUTOSERV', 'require_atfork_module', type=bool, default=False):
        print >>sys.stderr, 'Please run utils/build_externals.py'
        print e
        sys.exit(1)


The purposes of atfork are sort of mysterious, but it seems like it is intended to fix deadlocks that happen when you mix locks with *threads*. I think we have shunned python threads for development for several years, so not sure why we need this code (last touched in 2008). And this whacky behavior has caused at least 1 CQ failure in https://uberchromegw.corp.google.com/i/chromeos/builders/daisy_skate-paladin/builds/9142 due to bad interaction with another CL that set up some logging before atfork had a chance to attach.

 
Cc: sbasi@chromium.org
Looks like the main reason we were using this is when experimenting with paramiko (an ssh connection pooling library that I guess uses python threads).

From what I can tell from global_config and shadow_config, we do not use paramiko in prod. I vaguely recall that it was too buggy.
Cc: fdeng@chromium.org
+people who have touched paramiko
Summary: autoserv: stop using atfork + possibly delete paramiko host code (was: autoserv: stop using atfork)
FYI: Using multiprocessing and multithreading at the same time is pretty much impossible to get right on linux. Once you have multiple threads, forking becomes unsafe. It causes many common C libraries to misbehave, and even memlock had a deadlock bug in older libc...

From reading + experience, it's best to just avoid this.
+1 to #4 which is why we've shunned it for a long time.

Comment 6 by sbasi@chromium.org, Mar 1 2017

I remember Miller and I investigated Paramiko and we abandoned it in exchange for master ssh connections.

Feel free to delete all this relevant code.
Project Member

Comment 7 by bugdroid1@chromium.org, Mar 2 2017

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/chromeos-admin/+/96f8e17a3af6e387488ccc72951a332169411bfd

commit 96f8e17a3af6e387488ccc72951a332169411bfd
Author: Aviv Keshet <akeshet@chromium.org>
Date: Thu Mar 02 23:12:41 2017

Project Member

Comment 9 by bugdroid1@chromium.org, Mar 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/4e54ae7287b9601242e99dd6a8158d8b91824997

commit 4e54ae7287b9601242e99dd6a8158d8b91824997
Author: Aviv Keshet <akeshet@chromium.org>
Date: Thu Mar 02 23:12:41 2017

autotest: stop using atfork

CQ-DEPEND=CL:*332048
BUG= chromium:697254 
TEST=None

Change-Id: I3f3ae5d036e43248f7832059bccc70cb83a61263
Reviewed-on: https://chromium-review.googlesource.com/447864
Commit-Ready: Aviv Keshet <akeshet@chromium.org>
Tested-by: Aviv Keshet <akeshet@chromium.org>
Reviewed-by: Dan Shi <dshi@google.com>

[modify] https://crrev.com/4e54ae7287b9601242e99dd6a8158d8b91824997/utils/external_packages.py
[modify] https://crrev.com/4e54ae7287b9601242e99dd6a8158d8b91824997/server/autoserv
[modify] https://crrev.com/4e54ae7287b9601242e99dd6a8158d8b91824997/global_config.ini

Labels: -current-issue
Owner: akes...@chromium.org
Fixed now?
Status: Fixed (was: Untriaged)

Comment 12 by dchan@google.com, May 30 2017

Labels: VerifyIn-60
Labels: VerifyIn-61

Comment 14 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment