Page MenuHomePhabricator

Set a timeout for regex parsing in the Eventlogging processors
Open, Needs TriagePublic

Description

As a follow up for T200630, the best and most flexible solution seems to be to add a maximum execution time limit while parsing (applying various regexes) to the UA string.

An initial investigation from @Milimetric looks promising: https://gist.github.com/milimetric/2a9ee4eaad6d2a1523dece6166e2eb90

Event Timeline

elukey created this task.Jul 31 2018, 6:15 AM
elukey triaged this task as High priority.

Using a contextmanager decorator and the built-in TimeoutError exception (if Py3 only) the code could be much simpler, something like:

import signal

from contextlib import contextmanager


def timeout_handler(signum, frame):
    raise TimeoutError


@contextmanager
def timeout(seconds):
    prev_handler = signal.signal(signal.SIGALRM, timeout_handler)  # Store the previous signal handler
    signal.alarm(seconds)  # Set the timeout

    try:
        yield  # Yield to the context

    finally:
        signal.alarm(0)  # Reset the alarm
        signal.signal(signal.SIGALRM, prev_handler)  # Restore the previous handler


### Usage
try:
    with timeout(3):
        # call the ua parser
except TimeoutError:
    # handle the failed case

Thanks @Volans, didn't know about the contextmanager, that's nicer. And @elukey, as for concerns about the use of signal complicating the code, after reading more about it I think it's fine. Signals are received on the main thread as you said in the email, and the kafka-python threads are started in another part of the code. So all's good.

elukey removed elukey as the assignee of this task.Aug 24 2018, 1:46 PM
fdans moved this task from Incoming to Operational Excellence on the Analytics board.
Milimetric closed this task as Declined.Mar 4 2019, 4:52 PM

Closing because we haven't seen the issue resurface. Our temporary fix seems fine until we transition off of EventLogging, hopefully sooner than later.

Nuria added a comment.Mar 5 2019, 8:50 PM

Rather than closing I am going to move this one to "deprioritized" for now so as not to loose track of it completely.

Nuria reopened this task as Open.Mar 5 2019, 8:50 PM
Nuria lowered the priority of this task from High to Low.
Nuria moved this task from Operational Excellence to Deprioritized on the Analytics board.
Nuria raised the priority of this task from Low to Needs Triage.