Skip to content

amqp over websocket support - #197

Open
aloschilov wants to merge 11 commits into
mosquito:masterfrom
aloschilov:feature/websocket-transport-support
Open

amqp over websocket support#197
aloschilov wants to merge 11 commits into
mosquito:masterfrom
aloschilov:feature/websocket-transport-support

Conversation

@aloschilov

Copy link
Copy Markdown

This pull request provides support for using WebSocket as transport. Consequently, provided the mechanism for aio-pika to work.

@aloschilov

Copy link
Copy Markdown
Author

@mosquito It looks like it won't pass at least because of the very fact that github workflows should be modified. It requires https://github.com/cloudamqp/websocket-tcp-relay image. I have to play a little bit w.r.t. Github Actions to make it work.

@aloschilov

Copy link
Copy Markdown
Author

@mosquito adjusted to pass all the checks. I have to use docker compose instead of GHA services, since it is not possible to override entry point in an adequate way. See GitHub Workflows: Overriding A Service's Entrypoint:

The Benefits

Using a docker compose file instead of defining the entire build process in a Workflow loosens the coupling between your CI system and GitHub's Workflow platform.

In my case we'll probably be using Docker containers for a long time but who knows how long we'll stick with GitHub for our CI system. A docker compose should work on any platform that runs Docker, including your dev environment.

It also means that you can easily run your tests in a container that matches what'd be running in production. It reduces the risk of something working on GitHub's VM but blowing up in the container you're running on whatever infrastructure your production system is in.

So, if you can, just run a docker-compose from your action.

@ozobotnovako

Copy link
Copy Markdown
Contributor

Hi. I wanted to ask about the status of the pull request, as it's been idle for a while. I'm quite interested in the functionality it introduces and was wondering if there are any updates or if there's anything I can do to help move it forward.

Thanks!

@aloschilov

Copy link
Copy Markdown
Author

@ozobotnovako You could simplify checkout the specific branch and put the appropriate folder as a module. The order of resolution would do the trick.

$ pip install aio-pika 

This would install aiormq as dependency.

Then you could place aiormq as regular module to override original dependency.

<you project root>
|____aiormq
| |____auth.py
| |____tools.py
| |____channel.py
| |______init__.py
| |____types.py
| |____connection.py
| |____py.typed
| |____websocket_transport.py
| |____exceptions.py
| |____base.py
| |____abc.py
|____main.py

As for the MR, I suggest you test it first. Make a review at least. This would be beneficial. The very transport (from TCP to WebSocket) substitution could actually go to aiohttp and more importantly there is an appropriate issue for this.

@ozobotnovako

Copy link
Copy Markdown
Contributor

I did a quick test against RabbitMQ behind cloudamqp websocket relay, everything seems to be working as expected.

I couldn't find the relevant aiohttp issue though, could you please link it to the pr?

@aloschilov

Copy link
Copy Markdown
Author

@aloschilov

aloschilov commented Aug 23, 2024

Copy link
Copy Markdown
Author

@ozobotnovako

There is a pair of reader and writer in the context of aiohttp. The very idea was to modify aiohttp to make WebSocketReader, WebSocketWriter from aiohttp compatible with streams used in aiormq.

Then it would be one line solution for aiormq. This would simply mean that transport would go to aiohttp, which is a more fundamental modification.

class ClientWebSocketResponse:
    def __init__(
        self,
        reader: "FlowControlDataQueue[WSMessage]",
        writer: WebSocketWriter,
        protocol: Optional[str],
        response: ClientResponse,
        timeout: ClientWSTimeout,
        autoclose: bool,
        autoping: bool,
        loop: asyncio.AbstractEventLoop,
        *,
        heartbeat: Optional[float] = None,
        compress: int = 0,
        client_notakeover: bool = False,
    ) -> None:
...

@ozobotnovako

Copy link
Copy Markdown
Contributor

@aloschilov It would be nice to use the streaming API, but I'm not sure if it's possible to implement SocketReader/Writer API in a generic way. WebSockets support binary and text messages, while the streaming API only supports bytes. Therefore I think it would require a wrapper anyway.

However, in case it would make the PR more acceptable for @mosquito , I can try to open a discussion about that in the aiohttp repo.

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