Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose eventloop on Stream #290

Closed
wants to merge 3 commits into from

Conversation

ianopolous
Copy link
Contributor

@ianopolous ianopolous commented May 31, 2023

The use for the EventLoop is a p2p http proxy. It accepts incoming libp2p connections and proxies it to a fixed http endpoint or http handler. We needed access to the eventloop to add the new http handlers when receiving a new connection: https://github.com/Peergos/nabu/blob/master/src/main/java/org/peergos/protocol/http/HttpProtocol.java#L103

It is basically HTTP over libp2p. The entire implementation is in the single file linked above. We explain the use case a bit more in the "Decentralization" section of https://peergos.org/posts/dev-update

@Nashatyrev
Copy link
Collaborator

    public static void proxyRequest(Stream stream,
                                    HttpRequest msg,
                                    SocketAddress proxyTarget,
                                    Consumer<HttpObject> replyHandler) {
        Bootstrap b = new Bootstrap();
        b.group(stream.eventLoop())
                .channel(NioSocketChannel.class)
                .handler(new LoggingHandler(LogLevel.TRACE));

        ChannelFuture fut = b.connect(proxyTarget);
        Channel ch = fut.channel();
        ch.pipeline().addLast(new HttpRequestEncoder());
        ch.pipeline().addLast(new HttpResponseDecoder());
        ch.pipeline().addLast(new ResponseWriter(replyHandler));

        fut.addListener(x -> ch.writeAndFlush(msg));
    }

I'm not sure why do you need stream's eventLoop to create an independent connection?
Why not create a new NioEventLoopGroup or create and cache a single instance if it's performance critical?

@ianopolous
Copy link
Contributor Author

I wanted to reuse the same thread. We were originally using a new pool, but this was 1000X faster. However I've just benchmarked using a static instance and that doesn't seem any slower than the same thread. So I think you're right, we don't need this.

@ianopolous ianopolous closed this Jun 7, 2023
@Nashatyrev
Copy link
Collaborator

Nashatyrev commented Jun 8, 2023

Good, thanks for sorting this out 👍

A couple of notes on Netty code:

  • It's not safe adding handlers after connect() - it could be the case when data arrives earlier than handlers added and it would just be discarded
  • Need to double check if it's safe to invoke write() when connect() ChannelFuture completes. Basically the channel is ready for writing on channelActive event (see ChannelInboundHandler)

@Nashatyrev
Copy link
Collaborator

Sorry, accidentally posted incomplete comment. Some edit updates to previous comment

@Nashatyrev
Copy link
Collaborator

Was part of #272

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.

None yet

2 participants