Added timeout to kwargs for session.request

[timerke]

We can pass optional parameters to Redmine for the requests module. In particular, we can set timeout. But in fact, this timeout was not passed to the requests module, it was simply written to the attribute of the session object.

[maxtepkeev]

Can you please show your current code where you're trying to set timeout, thanks.

[timerke]

Timeout usage example:

"""
The example demonstrates how timeout works in the redmine and requests modules.
In the example you need to specify:
* URL - your Redmine location;
* API_KEY - your access key to Redmine API;
* PROJECT_NAME - the correct name of the project, information about which will be received from Redmine.

In the example, you first need to authenticate. After this, a message will appear indicating that you need to turn off
the Internet within a few seconds. After this, the redmine module will try to get project data. You need to pay
attention to the time it takes for a connection error to appear.
If the redmine module correctly passes timeout to the requests module, then the connection error should appear within
the time specified in the TIMEOUT constant.
"""

import time
from redminelib import Redmine
from requests.exceptions import ConnectionError, ReadTimeout


URL = ""  # please enter a correct URL
API_KEY = ""  # please enter a correct access key to Redmine API
PROJECT_NAME = ""  # please enter a correct project name

TIMEOUT = 4  # timeout in seconds for connecting and receiving data from Redmine

redmine = Redmine(URL, key=API_KEY, requests={"timeout": TIMEOUT})
user = redmine.auth()

SLEEP_TIME = 15
print(f"Wait {SLEEP_TIME} seconds. At this time you need to turn off your Internet connection")
time.sleep(SLEEP_TIME)

start_time = time.time()
try:
    print(f"Try to get information about project '{PROJECT_NAME}'...")
    users = redmine.project.get(PROJECT_NAME)
except (ConnectionError, ReadTimeout) as exc:
    print(f"Connection error: {exc}")
print(f"Time: {time.time() - start_time}")

In comments I tried to describe in detail what needs to be done to see the difference in the correct use of timeout.

Here is an example of setting timeout in the redmine module without changes And here is an example of setting timeout in the redmine module with changes

[maxtepkeev]

Thanks for providing such a thorough example.

I've investigated the issue and the timeout param is indeed not used by Requests in the way we currently set it, after digging a bit deeper into Requests code I was surprised, that Session object provides the ability to get and set almost all request method params as properties except for timeout and allow_redirects which seems super weird to me and isn't documented anywhere. After searching their Github I found several issues where people suggest that those two params should also be supported at session level, but all those issues were closed because Kenneth Reitz believes that we should use adapters for these params as they are more of a network level ones... I personally disagree with this, but anyway...

In terms of a suggested PR, I would do it a bit differently, how about we change the code in SyncEngine.create_session from the current:

session = requests.Session()

for param in params:
    setattr(session, param, params[param])

return session

to the following:

session = requests.Session
session.request = functools.partialmethod(session.request, **params)
return session()

So basically the sync.py file would look like this:

"""
Synchronous blocking engine that processes requests one by one.
"""

import functools
import requests

from . import BaseEngine


class SyncEngine(BaseEngine):
    @staticmethod
    def create_session(**params):
        session = requests.Session
        session.request = functools.partialmethod(session.request, **params)
        return session()

    def process_bulk_request(self, method, url, container, bulk_params):
        return [resource for params in bulk_params for resource in self.request(method, url, params=params)[container]]

This way we would also support not only timeout, but also allow_redirects and any other future param they could introduce to the requests method in the future.

Thoughts ?

[maxtepkeev]

Having thought about this a bit more, the implementation I suggested above isn't the best idea, as we basically monkey patch the Session.request method and this can cause problems for other libraries that can run simultaneously with Python-Redmine, so we need to think a bit more about it. Maybe create a session object first and then use functools.partial on the bound method instead of functool.partialmethod.

[timerke]

I think it's a good idea.

There is also this suggestion: You can pass kwargs directly to the request method of the Session class from the requests module. To do this, you only need to correct the construct_request_kwargs method of the BaseEngine class:

    def construct_request_kwargs(self, method, headers, params, data):
        """
        Constructs kwargs that will be used in all requests to Redmine.

        :param string method: (required). HTTP verb to use for the request.
        :param dict headers: (required). HTTP headers to send with the request.
        :param dict params: (required). Params to send in the query string.
        :param data: (required). Data to send in the body of the request.
        :type data: dict, bytes or file-like object
        """

        kwargs = {'data': data or {}, 'params': params or {}, 'headers': headers or {}, **self.requests}  # add the keyword arguments passed to Redmine object for the requests module

        if method in ('post', 'put', 'patch') and 'Content-Type' not in kwargs['headers']:
            kwargs['data'] = json.dumps(data)
            kwargs['headers']['Content-Type'] = 'application/json'

        return kwargs

[maxtepkeev]

Allright, let's do it your way, but we need to change the order in kwargs dict like this:

kwargs = dict(self.requests, **{'data': data or {}, 'params': params or {}, 'headers': headers or {}})

otherwise we can have problems.

Would you mind editing your PR ?

[timerke]

Edited

[maxtepkeev]

Awesome, thanks!