010 - pystalk: wrap raw connection errors in BeanstalkConnectionError by Adonis-Diaz · Pull Request #23 · EasyPost/pystalk

Adonis-Diaz · 2026-07-15T18:43:53Z

No description provided.

Steven Roomberg (sroomberg-ep)

After reading through these changes I think the best option is to use a decorator. Catching OSError is quite dangerous due to the large number of exceptions that encompasses.

Steven Roomberg (sroomberg-ep) · 2026-07-15T19:24:21Z

+        # __init__ expects a bytes ``message`` (it calls .decode('ascii')).
+        # We bypass it entirely: set our fields via object.__setattr__ (the
+        # class is frozen) and initialize the Exception base with a plain str.
+        object.__setattr__(self, 'host', host)


This is technically correct but way too verbose. The same thing can be done using the following

Suggested change

object.__setattr__(self, 'host', host)

self.host = host

self.port = port

self.original = original

Steven Roomberg (sroomberg-ep) · 2026-07-15T19:27:55Z

+        object.__setattr__(self, 'original', original)
+        msg = 'Failed to connect to beanstalkd at {0}:{1}: {2}'.format(host, port, original)
+        object.__setattr__(self, 'message', msg)
+        Exception.__init__(self, msg)


If this is a subclass of BeanstalkError you should be calling super for the parent class not Exception which is another level up. I'd probably add an init function to BeanstalkError since you're calling the implicit init function as well. Its best to use an explicit function rather than an implicit one.

Steven Roomberg (sroomberg-ep) · 2026-07-15T19:28:26Z

+            except OSError as e:
+                # OSError covers ConnectionRefusedError, socket.timeout,
+                # socket.error, and socket.gaierror in Python 3.
+                raise BeanstalkConnectionError(self.host, self.port, e) from e


you don't need from e

Steven Roomberg (sroomberg-ep) · 2026-07-15T19:32:58Z

+    ``except BeanstalkError`` callers continue to catch connection failures.
+    """
+
+    def __init__(self, host, port, original):


rather than original I'd just call it err since its more obvious what that means.

Steven Roomberg (sroomberg-ep) · 2026-07-15T19:42:48Z

+                self.socket = socket.create_connection(
+                    (self.host, self.port), timeout=self.socket_timeout
+                )
+            except OSError as e:


I think there is a better way of handling this implementation. Instead of running a try-except here I'd actually build out a decorator that you can define to catch any number of errors.

The decorator definition would look something like this (you will need to update it):

from functools import wraps def catch_and_raise(*errors, raise_as=BeanstalkConnectionError): """Catches specified errors and raises a specific exception instead.""" def decorator(func): @wraps(func) def wrapper(*args, **kwargs): try: return func(*args, **kwargs) except errors as e: print(f"[Decorator] Intercepted {type(e).__name__}") # Use 'from e' to preserve the original traceback (exception chaining) raise raise_as(f"Operation failed due to an internal error.") from e return wrapper return decorator

and using it would look like this:

@property @catch_and_raise(ConnectionRefusedError, socket.timeout, socket.error, socket.gaierror) def _socket(self): if self.socket is None: self.socket = socket.create_connection((self.host, self.port), timeout=self.socket_timeout) self._re_establish_use_watch() return self.socket

…class, rename original to err

… use super().__init__, f-string message

Steven Roomberg (sroomberg-ep)

Also add a description

Steven Roomberg (sroomberg-ep) · 2026-07-17T03:40:34Z

+    def __init__(self, message: Union[str, bytes]):
+        if isinstance(message, bytes):
+            message = message.decode('ascii')
+        object.__setattr__(self, 'message', message)


Just use

Suggested change

object.__setattr__(self, 'message', message)

self.message = message

Steven Roomberg (sroomberg-ep) · 2026-07-17T03:41:31Z

+    # BeanstalkError is an attr.s(frozen=True) class, and frozen __setattr__
+    # is inherited here too. Un-freeze this subclass so we can use plain
+    # attribute assignment below instead of object.__setattr__.
+    __setattr__ = object.__setattr__


You can get rid of this

Steven Roomberg (sroomberg-ep) · 2026-07-17T03:42:58Z

+            try:
+                return func(self, *args, **kwargs)
+            except errors as e:
+                raise raise_as(self.host, self.port, e) from e


You don't need from e

Suggested change

raise raise_as(self.host, self.port, e) from e

raise raise_as(self.host, self.port, e)

Steven Roomberg (sroomberg-ep) · 2026-07-17T03:44:12Z

        if self.socket is None:
-            self.socket = socket.create_connection((self.host, self.port), timeout=self.socket_timeout)
-            self._re_establish_use_watch()
+            self._connect()


I would not add this new function. You can use multiple decorators on a single function.

Steven Roomberg (sroomberg-ep) · 2026-07-17T15:21:01Z

+        if isinstance(message, bytes):
+            message = message.decode('ascii')
+        object.__setattr__(self, 'message', message)
+        Exception.__init__(self, message)


you can use super here

Steven Roomberg (sroomberg-ep) · 2026-07-17T15:22:43Z



-@attr.s(frozen=True, hash=True, eq=True)
+@attr.s(frozen=True, hash=True, eq=True, init=False)


I think this just complicates your life. Maybe we should consider getting rid of it entirely or at least getting rid of frozen=True

Steven Roomberg (sroomberg-ep)

A few minor, mostly cosmetic changes

Steven Roomberg (sroomberg-ep) · 2026-07-21T14:03:28Z

+        if isinstance(message, bytes):
+            message = message.decode('ascii')
+        self.message = message
+        super().__init__(message)


it is technically not required but I like the more verbose way of writing super()

Suggested change

super().__init__(message)

super(BeanstalkError, self).__init__(message)

Steven Roomberg (sroomberg-ep) · 2026-07-21T14:11:08Z

+        self.port = port
+        self.err = err
+        msg = f'Failed to connect to beanstalkd at {host}:{port}: {err}'
+        super().__init__(msg)


Same thing here for super()

There's no reason to store msg in a separate variable (this is again a stylistic thing for me personally so it doesn't matter if you change it or not).

For strings, even though python allows for single or double quotes, its generally better to use double quotes because, historically (in bash), single-quoted strings do not allow for interpolation (variable expansion - i.e., literal strings) whereas double-quoted strings allow for interpolation and therefore using them allows for a easier reading of code for people not familiar with how python works.

Suggested change

super().__init__(msg)

super(BeanstalkConnectionError, self).__init__(f"Failed to connect to beanstalkd at {host}:{port}: {err}")

Steven Roomberg (sroomberg-ep) · 2026-07-21T14:12:25Z



+def catch_and_raise(*errors, raise_as=BeanstalkConnectionError):
+    """Catches specified errors on an instance method and re-raises


More of a stylistic thing but I'd recommend multiline strings be structured with the opening/closing triple quotes on separate lines like so

""" Catches specified errors... """

Steven Roomberg (sroomberg-ep) · 2026-07-21T14:14:17Z

+    @catch_and_raise(ConnectionRefusedError, socket.timeout, socket.error, socket.gaierror)
    def _socket(self):
        if self.socket is None:
-            self.socket = socket.create_connection((self.host, self.port), timeout=self.socket_timeout)


There's no change here except the spacing. Did this line change due to a linter? If not, I'd revert this line back to

Suggested change

self.socket = socket.create_connection((self.host, self.port), timeout=self.socket_timeout)

self.socket = socket.create_connection((self.host, self.port), timeout=self.socket_timeout)

Steven Roomberg (sroomberg-ep)

I think you will need to bump the patch version and push a new tag before the merge occurs. I'll defer to Justin Hammond (@Justintime50) on this because I believe the publishing instructions need to be updated.

Justin Hammond (Justintime50)

You will need to bump the version in pystalk/__init__.py, the setup.py file, and then add an entry that matches describing your changes in docs/src/CHANGES.rst (I'd love to just move that to the root /CHANGES.rst, I have 0 idea why the indirection exists... but that's a separate issue to be handled later.

Once that's done, this is approved, and the code has been merged, you'll want to then cut a new release on GitHub and push to pypi. I am happy to assist with this, just let me know (it's highly possible we need to do some work around the releasing of this lib.) It would be advantageous for us to automate releasing via github actions like we do our python client library for customers. This should be done (probably first), separately from this PR.

Justin Hammond (Justintime50) · 2026-07-21T17:00:54Z

 version_info = (0, 7, 0)
 __version__ = '.'.join(str(s) for s in version_info)


As you'll need to bump the version, please simplify this and bump the version (note that version_info isn't used elsewhere so this is a safe change.

Suggested change

version_info = (0, 7, 0)

__version__ = '.'.join(str(s) for s in version_info)

__version__ = '0.8.0'

- Refactor _socket property: move connect logic into a decorated _connect() method so @Property no longer stacks with @catch_and_raise(...), fixing mypy's 'Decorated property not supported' error. - Remove whitespace-only blank line in BeanstalkConnectionError (flake8 W293/E303). - Remove trailing whitespace in test_pystalk.py (flake8 W291). - Bump setup.py version to 0.8.0 to match pystalk/__init__.py's __version__, which had drifted out of sync.

010 - pystalk: wrap raw connection errors in BeanstalkConnectionError

7948253

Adonis-Diaz added the bug label Jul 15, 2026

github-advanced-security AI found potential problems Jul 15, 2026

View reviewed changes

Comment thread tests/unit/test_pystalk.py Dismissed

Adonis-Diaz added enhancement and removed bug labels Jul 15, 2026

Steven Roomberg (sroomberg-ep) requested changes Jul 15, 2026

View reviewed changes

Adonis-Diaz added 2 commits July 16, 2026 11:20

address review feedback: use catch_and_raise decorator, un-freeze sub…

3812878

…class, rename original to err

address remaining review feedback: BeanstalkError gets explicit init,…

6adfcea

… use super().__init__, f-string message

Steven Roomberg (sroomberg-ep) requested changes Jul 17, 2026

View reviewed changes

Steven Roomberg (sroomberg-ep) reviewed Jul 17, 2026

View reviewed changes

Adonis-Diaz requested a review from Steven Roomberg (sroomberg-ep) July 17, 2026 17:25

Updated reviewer comments

9df6fe7

Steven Roomberg (sroomberg-ep) reviewed Jul 21, 2026

View reviewed changes

Reviewer comments corrections 2

7e696df

Steven Roomberg (sroomberg-ep) approved these changes Jul 21, 2026

View reviewed changes

Steven Roomberg (sroomberg-ep) requested changes Jul 21, 2026

View reviewed changes

Justin Hammond (Justintime50) requested changes Jul 21, 2026

View reviewed changes

Adonis-Diaz added 2 commits July 21, 2026 13:47

Version change

a9b2768

Adonis-Diaz changed the base branch from master to EXP-1217 July 23, 2026 20:02

Merge conflict

25d4b0e

	object.__setattr__(self, 'message', message)
	self.message = message

	raise raise_as(self.host, self.port, e) from e
	raise raise_as(self.host, self.port, e)



		@attr.s(frozen=True, hash=True, eq=True)
		@attr.s(frozen=True, hash=True, eq=True, init=False)

	super().__init__(message)
	super(BeanstalkError, self).__init__(message)

	super().__init__(msg)
	super(BeanstalkConnectionError, self).__init__(f"Failed to connect to beanstalkd at {host}:{port}: {err}")



		def catch_and_raise(*errors, raise_as=BeanstalkConnectionError):
		"""Catches specified errors on an instance method and re-raises

	self.socket = socket.create_connection((self.host, self.port), timeout=self.socket_timeout)
	self.socket = socket.create_connection((self.host, self.port), timeout=self.socket_timeout)

		version_info = (0, 7, 0)
		__version__ = '.'.join(str(s) for s in version_info)

	version_info = (0, 7, 0)
	__version__ = '.'.join(str(s) for s in version_info)
	__version__ = '0.8.0'

Uh oh!

Conversation

Adonis-Diaz commented Jul 15, 2026

Uh oh!

Uh oh!

Steven Roomberg (sroomberg-ep) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Steven Roomberg (sroomberg-ep) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Steven Roomberg (sroomberg-ep) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Steven Roomberg (sroomberg-ep) left a comment

Choose a reason for hiding this comment

Uh oh!

Justin Hammond (Justintime50) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants