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

Connection Pooling #51

Merged
merged 6 commits into from
Apr 19, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 18 additions & 1 deletion routemaster/app.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
"""Core App singleton that holds state for the application."""
import threading
from typing import Dict

from sqlalchemy.engine import Engine

from routemaster.db import initialise_db
from routemaster.config import Config
from routemaster.config import Config, StateMachine
from routemaster.logging import BaseLogger, SplitLogger, register_loggers
from routemaster.webhooks import (
WebhookRunner,
webhook_runner_for_state_machine,
)


class App(threading.local):
Expand All @@ -14,6 +19,7 @@ class App(threading.local):
db: Engine
config: Config
logger: BaseLogger
_webhook_runners: Dict[StateMachine, WebhookRunner]

def __init__(
self,
Expand All @@ -24,3 +30,14 @@ def __init__(
self.db = initialise_db(self.config.database)

self.logger = SplitLogger(config, loggers=register_loggers(config))

# Webhook runners may choose to persist a session, so we instantiate
# up-front to ensure we re-use state.
self._webhook_runners = {
x: webhook_runner_for_state_machine(y)
for x, y in self.config.state_machines.items()
}

def get_webhook_runner(self, state_machine: StateMachine) -> WebhookRunner:
"""Get the webhook runner for a state machine."""
return self._webhook_runners.get(state_machine.name)
11 changes: 9 additions & 2 deletions routemaster/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@
from routemaster.server import server
from routemaster.context import Context
from routemaster.logging import BaseLogger
from routemaster.webhooks import WebhookResult
from routemaster.webhooks import (
WebhookResult,
webhook_runner_for_state_machine,
)
from routemaster.state_machine import LabelRef
from routemaster.exit_conditions import ExitConditionProgram

Expand Down Expand Up @@ -228,6 +231,10 @@ def __init__(self, config):
self.config = config
self.database_used = False
self.logger = mock.MagicMock()
self._webhook_runners = {
x: webhook_runner_for_state_machine(y)
for x, y in self.config.state_machines.items()
}

@property
def db(self):
Expand Down Expand Up @@ -348,7 +355,7 @@ def mock_webhook():
def _mock(result=WebhookResult.SUCCESS):
runner = mock.Mock(return_value=result)
with mock.patch(
'routemaster.webhooks.RequestsWebhookRunner',
'routemaster.app.App.get_webhook_runner',
return_value=runner,
):
yield runner
Expand Down
9 changes: 8 additions & 1 deletion routemaster/feeds.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
"""Creation and fetching of feed data."""
import functools
from typing import Any, Dict, Callable, Optional

import requests
Expand All @@ -20,6 +21,11 @@ class FeedNotFetched(Exception):
pass


@functools.lru_cache()
Copy link

Choose a reason for hiding this comment

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

Using an explicit maxsize would be nice here, just saves a google to figure out what the connection pool size is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed this to a thread local anyway, but unfortunately this probably wouldn't work as there's another layer of indirection within requests to the actual connection pool. There doesn't seem to be an easy way to expose the actual connection pool parameters here.

def _get_feed_session():
return requests.Session()
Copy link
Contributor

Choose a reason for hiding this comment

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

These aren't thread-safe: I think this should be thread-local.

Copy link
Contributor Author

Choose a reason for hiding this comment

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



@dataclass
class Feed:
"""A feed fetcher, able to retreive a feed and read keys out of it."""
Expand All @@ -38,7 +44,8 @@ def prefetch(

url = template_url(self.url, self.state_machine, label)

response = requests.get(url)
session = _get_feed_session()
response = session.get(url)
log_response(response)
response.raise_for_status()
self.data = response.json()
Expand Down
7 changes: 2 additions & 5 deletions routemaster/state_machine/actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,7 @@
from routemaster.app import App
from routemaster.utils import template_url
from routemaster.config import State, Action, StateMachine
from routemaster.webhooks import (
WebhookResult,
webhook_runner_for_state_machine,
)
from routemaster.webhooks import WebhookResult
from routemaster.state_machine.types import LabelRef
from routemaster.state_machine.utils import (
choose_next_state,
Expand Down Expand Up @@ -58,7 +55,7 @@ def process_action(
'label': label.name,
}, sort_keys=True).encode('utf-8')

run_webhook = webhook_runner_for_state_machine(state_machine)
run_webhook = app.get_webhook_runner(state_machine)

idempotency_token = _calculate_idempotency_token(label, latest_history)

Expand Down
1 change: 1 addition & 0 deletions routemaster/tests/test_layering.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
('app', 'db'),
('app', 'config'),
('app', 'logging'),
('app', 'webhooks'),

('server', 'state_machine'),
('server', 'version'),
Expand Down
5 changes: 2 additions & 3 deletions routemaster/tests/test_webhook_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
from routemaster.webhooks import (
WebhookResult,
RequestsWebhookRunner,
webhook_runner_for_state_machine,
)


Expand Down Expand Up @@ -99,7 +98,7 @@ def test_requests_webhook_runner_for_state_machine_uses_webhook_config(app_confi
)

state_machine = app_config.config.state_machines['test_machine']
runner = webhook_runner_for_state_machine(state_machine)
runner = app_config.get_webhook_runner(state_machine)
runner('http://example.com', 'application/test-data', b'\0\xff', '')

last_request = httpretty.last_request()
Expand All @@ -117,7 +116,7 @@ def test_requests_webhook_runner_for_state_machine_does_not_apply_headers_for_no
)

state_machine = app_config.config.state_machines['test_machine']
runner = webhook_runner_for_state_machine(state_machine)
runner = app_config.get_webhook_runner(state_machine)
runner('http://not-example.com', 'application/test-data', b'\0\xff', '')

last_request = httpretty.last_request()
Expand Down