Skip to content

Conversation

@zerodefect
Copy link
Contributor

@zerodefect zerodefect commented Dec 9, 2025

This commit moves the file descriptor (fd) into a member variable _fd within the Watcher class.

Previously, fd was passed as an argument through every internal handler and captured in every lambda. Since the Watcher owns the socket, passing the fd explicitly was redundant. This change simplifies the function signatures and reduces the lambda capture overhead.

This commit updates the Boost Asio handler implementation to remove dependency on legacy Boost libraries in favor of modern C++17 standard features.

Changes include:
- Replaced all instances of `boost::bind` with native C++ lambdas.
- Removed `boost/bind/bind.hpp` dependency.
- Replaced `PTR_FROM_THIS` macro with standard `weak_from_this()`.
- Standardized callback types to use `std::function` instead of `boost::function`.
- Use `std::move()` semantics to reduce memory allocation(s).
This commit moves the file descriptor (`fd`) into a member variable `_fd`
within the `Watcher` class.

Previously, `fd` was passed as an argument through every internal handler
and captured in every lambda. Since the `Watcher` owns the socket, passing
the `fd` explicitly was redundant. This change simplifies the function
signatures and reduces the lambda capture overhead.
@zerodefect zerodefect changed the title Refactor/encapsulate fd Refactor: Encapsulate file descriptor in LibBoostAsioHandler Watcher class (pt 3) Dec 9, 2025
@paolopas
Copy link

paolopas commented Dec 17, 2025

Hi Zero,
Looking at the diff, there are a few things I think you should consider more carefully:

  1. for example in get_dispatch_wrapper you changed the error from operation_canceled to operation_aborted, right that's exactly what boost does on callbacks when they are cancelled. This seems like a rather unlikely scenario to me, if the handler cancels all callbacks during destruction.
  2. It's not accurate to say that the watcher has the socket. What the watcher has is a boost::asio::posix::stream_descriptor, which is initialized on the same file descriptor as the REAL socket that someone else has, just so it can observe it. They are two different objects; both can be closed or released independently.
  3. What never changes, however, is the connection pointer; perhaps that could have been saved in the watcher.

But what I think is that right now it's convenient to have both immediately available as parameters, and passing a pointer and an integer isn't a huge waste of resources. But above all, I think that before optimizing code, you need to clean up all the problems it has (and believe me, there are quite a few here), otherwise you'll just unnecessarily complicate the overall picture.

@zerodefect
Copy link
Contributor Author

For those interested, I've created a new repository which encapsulates an improved/updated implementation of LibBoostAsioHandler (aka LibBoostAsioHandler2).

https://github.com/coralbay-tv/amqp-cpp-boost2

PRs welcome :)

@paolopas
Copy link

@zerodefect, I think I explained it better here #550 (comment) how the handler works and how one should proceed with particular attention to the method that in your code is still called get_dispatch_wrapper but which instead I call make_handler into my version. I'm sure it will help you write a better version.

@zerodefect
Copy link
Contributor Author

@paolopas Thanks for the review and the feedback. I appreciate you taking the time to look through the diffs.

Regarding the error codes and ownership semantics, you raise some interesting architectural points. As I mentioned, I've moved the active development of this modern implementation to a dedicated repository (amqp-cpp-boost2) to allow for more significant refactoring without disrupting the legacy codebase here.

I'd be happy to continue the discussion there—if you have time, please feel free to open an issue or PR on that repo so we can track these specific improvements separately. For this PR, I'm aiming to keep the changes focused on the iterative cleanup we've already established.

@paolopas
Copy link

I'm sorry, but I see that you're badly copying the code you accessed, reintroducing errors that I had expertly eliminated, so it doesn't matter where you do it, in my opinion it's more important how. No one's "disrupting legacy code" here, no interface has changed, I'm just fixing all its many flaws. Problems that have waited years before I saw them...

@zerodefect
Copy link
Contributor Author

I understand you have a strong preference for your own implementation, but I am satisfied with the stability and trade-offs of this approach. I am freezing the scope of this PR here. If you believe your implementation is superior, I encourage you to submit your own PR to the maintainers for review. I will not be debating this further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants