Skip to content

Commit

Permalink
[ADD] external-request-timeout: It could wait for a long time (#370)
Browse files Browse the repository at this point in the history 
Calling external request needs to use a timeout in order to avoid waiting for a long time
It could be even worse if you have one or many records locked
then calling a request it could accumulate a lot of locks and
the workers could be used for more time and the system could be down
waiting for release

You can define your custom methods using the parameter: 
`--external_request_timeout_methods`

The default methods are:
    - http.client.HTTPConnection
    - http.client.HTTPSConnection
    - odoo.addons.iap.models.iap.jsonrpc
    - requests.delete
    - requests.get
    - requests.head
    - requests.options
    - requests.patch
    - requests.post
    - requests.put
    - requests.request
    - serial.Serial
    - smtplib.SMTP
    - suds.client.Client
    - urllib.request.urlopen
  • Loading branch information
@moylop260
moylop260 committed May 24, 2022
1 parent d260411 commit 4b6fcbe
Show file tree
Hide file tree
Showing 3 changed files with 242 additions and 1 deletion.
79 changes: 78 additions & 1 deletion pylint_odoo/checkers/no_modules.py
View file
Expand Up @@ -23,6 +23,7 @@
visit_expr
visit_extslice
visit_for
visit_import
visit_importfrom
visit_functiondef
visit_genexpr
Expand Down Expand Up @@ -144,6 +145,12 @@
'str-format-used',
settings.DESC_DFLT
),
'E%d06' % settings.BASE_NOMODULE_ID: (
'Use of external request method `%s` without timeout. '
'It could wait for a long time',
'external-request-timeout',
settings.DESC_DFLT
),
'C%d01' % settings.BASE_NOMODULE_ID: (
'One of the following authors must be present in manifest: %s',
'manifest-required-author',
Expand Down Expand Up @@ -302,6 +309,23 @@
# From odoo/odoo 10.0: odoo/odoo/fields.py:29
'digits_compute:digits', 'select:index'
]
DFTL_EXTERNAL_REQUEST_TIMEOUT_METHODS = [
"http.client.HTTPConnection",
"http.client.HTTPSConnection",
"odoo.addons.iap.models.iap.jsonrpc",
"requests.delete",
"requests.get",
"requests.head",
"requests.options",
"requests.patch",
"requests.post",
"requests.put",
"requests.request",
"serial.Serial",
"smtplib.SMTP",
"suds.client.Client",
"urllib.request.urlopen",
]


class NoModuleChecker(misc.PylintOdooChecker):
Expand Down Expand Up @@ -413,8 +437,29 @@ class NoModuleChecker(misc.PylintOdooChecker):
'help': 'List of valid missing return method names, '
'separated by a comma.'
}),
('external_request_timeout_methods', {
'type': 'csv',
'metavar': '<comma separated values>',
'default': DFTL_EXTERNAL_REQUEST_TIMEOUT_METHODS,
'help': 'List of library.method that must have a timeout '
'parameter defined, separated by a comma. '
'e.g. "requests.get,requests.post"'
}),
)

def visit_module(self, node):
"""Initizalize the cache to save the original library name
of all imported node
It is filled from "visit_importfrom" and "visit_import"
and it is used in "visit_call"
All these methods are these "visit_*" methods are called from pylint API
"""
self._from_imports = {}

def leave_module(self, node):
"""Clear variables"""
self._from_imports = {}

def open(self):
super(NoModuleChecker, self).open()
self.config.deprecated_field_parameters = \
Expand Down Expand Up @@ -589,6 +634,7 @@ def visit_print(self, node):
'translation-contains-variable',
'print-used', 'translation-positional-used',
'str-format-used', 'context-overridden',
'external-request-timeout',
)
def visit_call(self, node):
infer_node = utils.safe_infer(node.func)
Expand Down Expand Up @@ -750,6 +796,27 @@ def visit_call(self, node):
if self._check_sql_injection_risky(node):
self.add_message('sql-injection', node=node)

# external-request-timeout
lib_alias = self.get_func_lib(node.func)
# Use dict "self._from_imports" to know the source library of the method
lib_original = self._from_imports.get(lib_alias) or lib_alias
func_name = self.get_func_name(node.func)
lib_original_func_name = (
# If it using "requests.request()"
"%s.%s" % (lib_original, func_name) if lib_original
# If it using "from requests import request;request()"
else self._from_imports.get(func_name))
if lib_original_func_name in self.config.external_request_timeout_methods:
for argument in misc.join_node_args_kwargs(node):
if not isinstance(argument, astroid.Keyword):
continue
if argument.arg == 'timeout':
break
else:
self.add_message(
'external-request-timeout', node=node,
args=(lib_original_func_name,))

@utils.check_messages(
'license-allowed', 'manifest-author-string', 'manifest-deprecated-key',
'manifest-required-author', 'manifest-required-key',
Expand Down Expand Up @@ -911,12 +978,22 @@ def visit_functiondef(self, node):
node.name not in self.config.no_missing_return:
self.add_message('missing-return', node=node, args=(node.name))

@utils.check_messages('openerp-exception-warning')
@utils.check_messages('external-request-timeout')
def visit_import(self, node):
self._from_imports.update({
alias or name: "%s" % name
for name, alias in node.names
})

@utils.check_messages('openerp-exception-warning', 'external-request-timeout')
def visit_importfrom(self, node):
if node.modname == 'openerp.exceptions':
for (import_name, import_as_name) in node.names:
if import_name == 'Warning' and import_as_name != 'UserError':
self.add_message('openerp-exception-warning', node=node)
self._from_imports.update({
alias or name: "%s.%s" % (node.modname, name)
for name, alias in node.names})

@utils.check_messages('class-camelcase')
def visit_classdef(self, node):
Expand Down
1 change: 1 addition & 0 deletions pylint_odoo/test/main.py
View file
Expand Up @@ -30,6 +30,7 @@
'duplicate-po-message-definition': 2,
'duplicate-xml-fields': 9,
'duplicate-xml-record-id': 2,
'external-request-timeout': 47,
'file-not-used': 6,
'incoherent-interpreter-exec-perm': 3,
'invalid-commit': 4,
Expand Down
163 changes: 163 additions & 0 deletions pylint_odoo/test_repo/broken_module/models/broken_model.py
View file
Expand Up @@ -3,6 +3,38 @@
import psycopg2
from psycopg2 import sql
from psycopg2.sql import SQL, Identifier
import requests
from requests import (
delete, get, head, options, patch, post, put, request)
from requests import (
delete as delete_r, get as get_r, head as head_r,
options as options_r, patch as patch_r, post as post_r,
put as put_r, request as request_r)

import urllib
from urllib.request import urlopen
from urllib.request import urlopen as urlopen_r

import suds.client
from suds.client import Client
from suds.client import Client as Client_r

import http.client
from http.client import HTTPConnection, HTTPSConnection
from http.client import (
HTTPConnection as HTTPConnection_r,
HTTPSConnection as HTTPSConnection_r,
)

import smtplib
import smtplib as smtplib_r
from smtplib import SMTP
from smtplib import SMTP as SMTP_r

import serial
import serial as serial_r
from serial import Serial
from serial import Serial as Serial_r

from openerp import fields, models, _
from openerp.exceptions import Warning as UserError
Expand All @@ -14,6 +46,10 @@
import openerp.addons.broken_module as broken_module2
import openerp.addons.broken_module.broken_model as broken_model2

import odoo.addons.iap.models.iap.jsonrpc
from odoo.addons.iap.models.iap import jsonrpc
from odoo.addons.iap.models import iap


other_field = fields.Char()

Expand Down Expand Up @@ -537,6 +573,133 @@ def func(self, a):
length = len(a)
return length

def requests_test(self):
# requests without timeout
requests.delete('http://localhost')
requests.get('http://localhost')
requests.head('http://localhost')
requests.options('http://localhost')
requests.patch('http://localhost')
requests.post('http://localhost')
requests.put('http://localhost')
requests.request('call', 'http://localhost')

delete_r('http://localhost')
get_r('http://localhost')
head_r('http://localhost')
options_r('http://localhost')
patch_r('http://localhost')
post_r('http://localhost')
put_r('http://localhost')
request_r('call', 'http://localhost')

delete('http://localhost')
get('http://localhost')
head('http://localhost')
options('http://localhost')
patch('http://localhost')
post('http://localhost')
put('http://localhost')
request('call', 'http://localhost')

# requests valid cases
requests.delete('http://localhost', timeout=10)
requests.get('http://localhost', timeout=10)
requests.head('http://localhost', timeout=10)
requests.options('http://localhost', timeout=10)
requests.patch('http://localhost', timeout=10)
requests.post('http://localhost', timeout=10)
requests.put('http://localhost', timeout=10)
requests.request('call', 'http://localhost', timeout=10)

delete_r('http://localhost', timeout=10)
get_r('http://localhost', timeout=10)
head_r('http://localhost', timeout=10)
options_r('http://localhost', timeout=10)
patch_r('http://localhost', timeout=10)
post_r('http://localhost', timeout=10)
put_r('http://localhost', timeout=10)
request_r('call', 'http://localhost', timeout=10)

delete('http://localhost', timeout=10)
get('http://localhost', timeout=10)
head('http://localhost', timeout=10)
options('http://localhost', timeout=10)
patch('http://localhost', timeout=10)
post('http://localhost', timeout=10)
put('http://localhost', timeout=10)
request('call', 'http://localhost', timeout=10)

# urllib without timeout
urllib.request.urlopen('http://localhost')
urlopen('http://localhost')
urlopen_r('http://localhost')

# urllib valid cases
urllib.request.urlopen('http://localhost', timeout=10)
urlopen('http://localhost', timeout=10)
urlopen_r('http://localhost', timeout=10)

# suds without timeout
suds.client.Client('http://localhost')
Client('http://localhost')
Client_r('http://localhost')

# suds valid cases
suds.client.Client('http://localhost', timeout=10)
Client('http://localhost', timeout=10)
Client_r('http://localhost', timeout=10)

# http.client without timeout
http.client.HTTPConnection('http://localhost')
http.client.HTTPSConnection('http://localhost')
HTTPConnection('http://localhost')
HTTPSConnection('http://localhost')
HTTPConnection_r('http://localhost')
HTTPSConnection_r('http://localhost')

# http.client valid cases
http.client.HTTPConnection('http://localhost', timeout=10)
http.client.HTTPSConnection('http://localhost', timeout=10)
HTTPConnection('http://localhost', timeout=10)
HTTPSConnection('http://localhost', timeout=10)
HTTPConnection_r('http://localhost', timeout=10)
HTTPSConnection_r('http://localhost', timeout=10)

# smtplib without timeout
smtplib.SMTP('http://localhost')
smtplib_r.SMTP('http://localhost')
SMTP('http://localhost')
SMTP_r('http://localhost')

# smtplib valid cases
smtplib.SMTP('http://localhost', timeout=10)
smtplib_r.SMTP('http://localhost', timeout=10)
SMTP('http://localhost', timeout=10)
SMTP_r('http://localhost', timeout=10)

# Serial without timeout
serial.Serial('/dev/ttyS1')
serial_r.Serial('/dev/ttyS1')
Serial('/dev/ttyS1')
Serial_r('/dev/ttyS1')

# serial valid cases
serial.Serial('/dev/ttyS1', timeout=10)
serial_r.Serial('/dev/ttyS1', timeout=10)
Serial('/dev/ttyS1', timeout=10)
Serial_r('/dev/ttyS1', timeout=10)

# odoo.addons.iap without timeout
odoo.addons.iap.models.iap.jsonrpc('http://localhost')
jsonrpc('http://localhost')
iap.jsonrpc('http://localhost')

# odoo.addons.iap valid cases
odoo.addons.iap.models.iap.jsonrpc('http://localhost', timeout=10)
jsonrpc('http://localhost', timeout=10)
iap.jsonrpc('http://localhost', timeout=10)


class NoOdoo(object):
length = 0
Expand Down

0 comments on commit 4b6fcbe

Please sign in to comment.