allow getting recoverable_connection_errors without an active transport #1471

pawl · 2021-12-22T01:41:20Z

I was looking into solutions for the celery + redis memory leak issue and I was attempting to reduce the references to initialized Transports.

One thing I noticed was that you currently need at least one initialized Transport with an active connection to allow filling the cache for recoverable_connection_errors/recoverable_channel_errors/connection_errors/channel_errors. It seems like it makes more sense to get the errors from the Transport class without needing to initialize it (in case there are connection errors on the first Transport initialization).

This doesn't fix the memory leak issue, but maybe this change is still worth making?

lgtm-com · 2021-12-22T01:50:56Z

This pull request fixes 1 alert when merging c217795 into b71b421 - view on LGTM.com

fixed alerts:

1 for `__init__` method calls overridden method

auvipy · 2021-12-22T02:46:41Z

kombu/transport/consul.py

-        super().__init__(*args, **kwargs)
-
-        self.connection_errors = (
+    if consul:


only 3 transports are subject to this adjustments?

Yeah, I did a search for self.connection_errors and self.channel_errors, and looked at each of the transports.

For some reason, the librabbitmq and mongodb backends don't have the try/except ImportError: logic and they don't use __init__ to fill in connection_errors and channel_errors.

auvipy · 2021-12-22T02:48:20Z

t/unit/test_connection.py

@@ -571,6 +573,16 @@ class MyTransport(Transport):
        conn = Connection(transport=MyTransport)
        assert conn.channel_errors == (KeyError, ValueError)

+    def test_channel_errors__exception_no_cache(self):


beside the unit tests I would love to see integration tests for this

…r_first_attempt

lgtm-com · 2021-12-22T03:21:35Z

This pull request fixes 1 alert when merging a08d2ad into c4a8c2a - view on LGTM.com

fixed alerts:

1 for `__init__` method calls overridden method

…r_first_attempt

lgtm-com · 2021-12-22T03:40:17Z

This pull request fixes 1 alert when merging a8a9913 into 6ddf940 - view on LGTM.com

fixed alerts:

1 for `__init__` method calls overridden method

auvipy

with pytest.raises(KeyError):
self.channel._receive_one(self.channel.subclient)

  assert not self.channel._in_listen

E assert not True
E + where True = <t.unit.transport.test_redis.Channel object at 0x7f8bf8d594d0>._in_listen
E + where <t.unit.transport.test_redis.Channel object at 0x7f8bf8d594d0> = <t.unit.transport.test_redis.test_Channel object at 0x7f8bf8d591d0>.channel

t/unit/transport/test_redis.py:582: AssertionError

for more information, see https://pre-commit.ci

lgtm-com · 2021-12-22T04:08:03Z

This pull request fixes 1 alert when merging c2c6b1b into 4a6e164 - view on LGTM.com

fixed alerts:

1 for `__init__` method calls overridden method

lgtm-com · 2021-12-22T04:36:50Z

This pull request fixes 1 alert when merging c6052ff into 4a6e164 - view on LGTM.com

fixed alerts:

1 for `__init__` method calls overridden method

auvipy

I would like to take some time & more eyes and trials before merging this. in the mean the you can push any improvements in mind. and most possibly the more integration tests the better IMHO

auvipy · 2021-12-22T04:45:29Z

besides, do we need any docs changes to mention the adjustments being made?

auvipy · 2021-12-22T04:53:20Z

@michael-lazar

auvipy · 2021-12-24T03:40:40Z

@pawl did you try this in any project with celery? i want to have some integration test for this

lgtm-com · 2021-12-24T04:01:36Z

This pull request fixes 1 alert when merging 448cdb9 into 47781af - view on LGTM.com

fixed alerts:

1 for `__init__` method calls overridden method

pawl · 2021-12-24T04:09:50Z

@auvipy I added an integration test here: 448cdb9

do we need any docs changes to mention the adjustments being made?

Oops, should I be updating the changelog?

I'm not aware of any documentation changes since old code should continue to work the same way, because the errors are still a class attribute on the same class.

I'm not sure we need to document the removal of _get_errors outside of the release notes since it's an internal method. It also doesn't look like it's overridden in any open source projects: https://github.com/search?q=transport+_get_errors+NOT+MultiChannelPoller+NOT+test_purge+NOT+%22by+Ask+Solem%22&type=Code

pawl · 2021-12-24T04:34:56Z

@auvipy

did you try this in any project with celery?

I've tested the change with this branch of my celery memory leak example repo. It's still able to recover from connection errors successfully after I restart redis. Also, it connects successfully on start-up.

auvipy · 2021-12-24T05:02:27Z

ok we are good to go

auvipy · 2021-12-24T05:35:05Z

decided to land this change on 5.3 for now, as it changes some behaviours, but if other core team members agree will end landing in 5.2.x bug fix release

kombu/connection.py

matusvalo

I am not 100% sure but if I read correctly the code get_transport_cls() needs a transport as string or type. Default value is None which I am not sure will work.

kombu/kombu/transport/__init__.py

Lines 47 to 86 in b6b4408

    
           def resolve_transport(transport=None): 
        
               """Get transport by name. 
        
               Arguments: 
        
                   transport (Union[str, type]): This can be either 
        
                       an actual transport class, or the fully qualified 
        
                       path to a transport class, or the alias of a transport. 
        
               """ 
        
               if isinstance(transport, str): 
        
                   try: 
        
                       transport = TRANSPORT_ALIASES[transport] 
        
                   except KeyError: 
        
                       if '.' not in transport and ':' not in transport: 
        
                           from kombu.utils.text import fmatch_best 
        
                           alt = fmatch_best(transport, TRANSPORT_ALIASES) 
        
                           if alt: 
        
                               raise KeyError( 
        
                                   'No such transport: {}.  Did you mean {}?'.format( 
        
                                       transport, alt)) 
        
                           raise KeyError(f'No such transport: {transport}') 
        
                   else: 
        
                       if callable(transport): 
        
                           transport = transport() 
        
                   return symbol_by_name(transport) 
        
               return transport 
        
           def get_transport_cls(transport=None): 
        
               """Get transport class by name. 
        
               The transport string is the full path to a transport class, e.g.:: 
        
                   "kombu.transport.pyamqp:Transport" 
        
               If the name does not include `"."` (is not fully qualified), 
        
               the alias table will be consulted. 
        
               """ 
        
               if transport not in _transport_cache: 
        
                   _transport_cache[transport] = resolve_transport(transport) 
        
               return _transport_cache[transport]

kombu/connection.py

pawl · 2021-12-29T22:49:50Z

@matusvalo get_transport_cls is already used without arguments in create_transport (used by the transport property):

kombu/kombu/connection.py

Line 606 in a9c4f98

return self.get_transport_cls()(client=self)

It looks like _init_params will populate self.transport_cls and it will be used by get_transport_cls.

matusvalo · 2021-12-29T23:51:42Z

@matusvalo get_transport_cls is already used without arguments in create_transport (used by the transport property):

kombu/kombu/connection.py

Line 606 in a9c4f98

return self.get_transport_cls()(client=self)

It looks like _init_params will populate self.transport_cls and it will be used by get_transport_cls.

Yeah you are right :-/. I missed that it is wrapped in the same class. Please ignore my comment.

…rt (celery#1471) * allow getting recoverable_connection_errors without an active transport * move redis transport errors to class * move consul transport errors to class * move etcd transport errors to class * remove redis.Transport._get_errors and references in tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * fix flake8 errors * add integration test for redis ConnectionError Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

pawl added 4 commits December 21, 2021 18:45

allow getting recoverable_connection_errors without an active transport

33f8923

move redis transport errors to class

46462b9

move consul transport errors to class

b6591e1

move etcd transport errors to class

c217795

auvipy requested changes Dec 22, 2021

View reviewed changes

Merge branch 'master' of github.com:celery/kombu into connection_erro…

a08d2ad

…r_first_attempt

Merge branch 'master' of github.com:celery/kombu into connection_erro…

a8a9913

…r_first_attempt

auvipy requested changes Dec 22, 2021

View reviewed changes

pawl and others added 2 commits December 21, 2021 21:57

remove redis.Transport._get_errors and references in tests

bc7b8ba

[pre-commit.ci] auto fixes from pre-commit.com hooks

c2c6b1b

for more information, see https://pre-commit.ci

fix flake8 errors

c6052ff

auvipy added this to the 5.2.x milestone Dec 22, 2021

auvipy requested a review from matusvalo December 22, 2021 04:42

auvipy requested changes Dec 22, 2021

View reviewed changes

add integration test for redis ConnectionError

448cdb9

auvipy approved these changes Dec 24, 2021

View reviewed changes

auvipy added PR Type: Bugfix PR Type: Enhancement labels Dec 24, 2021

auvipy modified the milestones: 5.2.x, 5.3 Dec 24, 2021

matusvalo reviewed Dec 29, 2021

View reviewed changes

kombu/connection.py Show resolved Hide resolved

matusvalo requested changes Dec 29, 2021

View reviewed changes

kombu/connection.py Show resolved Hide resolved

kombu/connection.py Show resolved Hide resolved

matusvalo approved these changes Dec 29, 2021

View reviewed changes

auvipy modified the milestones: 5.3, 5.2.x Dec 30, 2021

auvipy merged commit 9c062bd into celery:master Dec 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow getting recoverable_connection_errors without an active transport #1471

allow getting recoverable_connection_errors without an active transport #1471

pawl commented Dec 22, 2021 •

edited

Loading

lgtm-com bot commented Dec 22, 2021

auvipy Dec 22, 2021

pawl Dec 22, 2021

auvipy Dec 22, 2021

lgtm-com bot commented Dec 22, 2021

lgtm-com bot commented Dec 22, 2021

auvipy left a comment

lgtm-com bot commented Dec 22, 2021

lgtm-com bot commented Dec 22, 2021

auvipy left a comment

auvipy commented Dec 22, 2021

auvipy commented Dec 22, 2021 •

edited

Loading

auvipy commented Dec 24, 2021

lgtm-com bot commented Dec 24, 2021

pawl commented Dec 24, 2021

pawl commented Dec 24, 2021 •

edited

Loading

auvipy commented Dec 24, 2021

auvipy commented Dec 24, 2021

matusvalo left a comment

pawl commented Dec 29, 2021

matusvalo commented Dec 29, 2021

	def resolve_transport(transport=None):
	"""Get transport by name.

	Arguments:
	transport (Union[str, type]): This can be either
	an actual transport class, or the fully qualified
	path to a transport class, or the alias of a transport.
	"""
	if isinstance(transport, str):
	try:
	transport = TRANSPORT_ALIASES[transport]
	except KeyError:
	if '.' not in transport and ':' not in transport:
	from kombu.utils.text import fmatch_best
	alt = fmatch_best(transport, TRANSPORT_ALIASES)
	if alt:
	raise KeyError(
	'No such transport: {}. Did you mean {}?'.format(
	transport, alt))
	raise KeyError(f'No such transport: {transport}')
	else:
	if callable(transport):
	transport = transport()
	return symbol_by_name(transport)
	return transport


	def get_transport_cls(transport=None):
	"""Get transport class by name.

	The transport string is the full path to a transport class, e.g.::

	"kombu.transport.pyamqp:Transport"

	If the name does not include `"."` (is not fully qualified),
	the alias table will be consulted.
	"""
	if transport not in _transport_cache:
	_transport_cache[transport] = resolve_transport(transport)
	return _transport_cache[transport]

allow getting recoverable_connection_errors without an active transport #1471

allow getting recoverable_connection_errors without an active transport #1471

Conversation

pawl commented Dec 22, 2021 • edited Loading

lgtm-com bot commented Dec 22, 2021

auvipy Dec 22, 2021

Choose a reason for hiding this comment

pawl Dec 22, 2021

Choose a reason for hiding this comment

auvipy Dec 22, 2021

Choose a reason for hiding this comment

lgtm-com bot commented Dec 22, 2021

lgtm-com bot commented Dec 22, 2021

auvipy left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Dec 22, 2021

lgtm-com bot commented Dec 22, 2021

auvipy left a comment

Choose a reason for hiding this comment

auvipy commented Dec 22, 2021

auvipy commented Dec 22, 2021 • edited Loading

auvipy commented Dec 24, 2021

lgtm-com bot commented Dec 24, 2021

pawl commented Dec 24, 2021

pawl commented Dec 24, 2021 • edited Loading

auvipy commented Dec 24, 2021

auvipy commented Dec 24, 2021

matusvalo left a comment

Choose a reason for hiding this comment

pawl commented Dec 29, 2021

matusvalo commented Dec 29, 2021

pawl commented Dec 22, 2021 •

edited

Loading

auvipy commented Dec 22, 2021 •

edited

Loading

pawl commented Dec 24, 2021 •

edited

Loading