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

http: Make it possible to pass in a pre-existing connection #479

Closed
wants to merge 2 commits into from
Closed

http: Make it possible to pass in a pre-existing connection #479

wants to merge 2 commits into from

Conversation

laanwj
Copy link

@laanwj laanwj commented Mar 4, 2017

This patch adds functionality to pass a pre-existing connection as a bufferevent to evhttp_connection_base_bufferevent_new. As far as I know, this was not possible before.

When the bufferevent has an existing fd, the evcon starts in state EVCON_IDLE so that requests can be made immediately.

I am using this in Bitcoin Core for doing HTTP requests over a UNIX socket (bitcoin/bitcoin#9919).

It could also be useful in sandboxed environments such as CloudABI that cannot directly make connections but can pass around fds.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 80.48% when pulling 403b793 on laanwj:2017_03_evhttp_existing_fd into cc0e04d on libevent:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 80.48% when pulling 403b793 on laanwj:2017_03_evhttp_existing_fd into cc0e04d on libevent:master.

@azat azat mentioned this pull request Apr 22, 2017
@azat
Copy link
Member

azat commented Apr 22, 2017

@laanwj sorry for the huge delay, patch LGTM, but with it evhttp_connection_set_retries will be no-op, even though this will work for you it will make things tricky for others.

That's why I hadn't merged it already. But I had an idea of how we can make it works for everybody. What do you think?

@ghazel
Copy link
Contributor

ghazel commented May 21, 2017

You'll also want to set evcon->flags |= EVHTTP_CON_OUTGOING; so that the connection can read more than one request.

@laanwj
Copy link
Author

laanwj commented May 22, 2017

but with it evhttp_connection_set_retries will be no-op, even though this will work for you it will make things tricky for others.

I figured automatic retries aren't really a possibility if the application provides the connection. This uses libevent only for the protocol handling. If the application needs retries, it has to implement them itself.

evcon->flags |= EVHTTP_CON_OUTGOING; so that the connection can read more than one request.

Good idea.

@laanwj
Copy link
Author

laanwj commented Jun 29, 2017

I added EVHTTP_CON_OUTGOING. Making the retry work with a callback (at either http or bufferevent level) as suggested by @azat sounds like a good idea to me, but that exceeds my expertise with the libevent code.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 80.418% when pulling 08e0114 on laanwj:2017_03_evhttp_existing_fd into cc0e04d on libevent:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 80.418% when pulling 08e0114 on laanwj:2017_03_evhttp_existing_fd into cc0e04d on libevent:master.

This patch adds functionality to pass a pre-existing connection
as a bufferevent to `evhttp_connection_base_bufferevent_new`.

When the bufferevent has an existing fd, the evcon starts
in state `EVCON_IDLE` so that requests can be made immediately.

I am using this in Bitcoin Core for doing HTTP requests over
a UNIX socket (bitcoin/bitcoin#9919).

It could also be useful in sandboxed environments such as cloudabi
that cannot directly make connections but can pass around fds.
Suggested by Greg Hazel so the connection can read more than one
request.
@laanwj
Copy link
Author

laanwj commented Feb 8, 2021

I'm not entirely sure what is blocking this, but will be looking at a different solution for this bypassing libevent.

@laanwj laanwj closed this Feb 8, 2021
@azat
Copy link
Member

azat commented Feb 9, 2021

I'm not entirely sure what is blocking this, but will be looking at a different solution for this bypassing libevent.

@laanwj sorry for my ignorance, last time I looked at this I did not like that this makes no-op some of functionality (evhttp_connection_set_retries), and also that fact that http layer is not in a good shape.

I'm OK to merge this change if:

  • it will be done under some connection flag (evhttp_connection_set_flags)
  • it will have a test (in test/regress_http.c)

@laanwj
Copy link
Author

laanwj commented Feb 9, 2021

Thanks for the elaboration!

I can't guarantee to get around to that but will keep it in mind.

(it's not that I really insist on this solution, I just need something lightweight to connect to a HTTP server on a UNIX oscket 🙂 )

@azat
Copy link
Member

azat commented Feb 9, 2021

it's not that I really insist on this solution

It may be useful to inject something into http layer indeed (and not only for unix sockets)

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

Successfully merging this pull request may close these issues.

None yet

4 participants