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

Discussion: should HTTP/2 pool skip over not ready connections? #216

Open
Goose97 opened this issue Feb 28, 2023 · 3 comments · May be fixed by #227
Open

Discussion: should HTTP/2 pool skip over not ready connections? #216

Goose97 opened this issue Feb 28, 2023 · 3 comments · May be fixed by #227

Comments

@Goose97
Copy link

Goose97 commented Feb 28, 2023

We sometimes encounter this error in our production release:

{:error, %Finch.Error{reason: :read_only}}

This error indicates that the checked out pool is close for writing and only able to read responses. If the pool is in :disconnected or :connected_read_only state, it's not able to make requests (I'll refer these two states as not_ready from now on). This problem is worsen if the reconnect process takes longer, eg: when network connections are unstable.

My point is we should be able to filter out not_ready pools while performing lookup. Right now, there's no way to efficiently get pool's current state other than sending a message to it (GenServer.call, :sys.get_state, ...). How about lifting the pool state to an ets table?

@sneako
Copy link
Owner

sneako commented Mar 3, 2023

I agree, when we lookup a pool/connection (for http2 they are the same thing), we should not return connections that are not ready for use. Maybe the connection should unregister itself from the Registry whenever it is not ready to accept new requests, and re-registry itself once it is ready again?

@Goose97
Copy link
Author

Goose97 commented Mar 7, 2023

cool, this should work. I'll try a PR soon.

Goose97 added a commit to Goose97/finch that referenced this issue May 25, 2023
HTTP2 connections will unregister themselves from the pool once enter connected_read_only and disconnected states, and register themselves once they reconnect.

Fixes sneako#216
@Goose97 Goose97 linked a pull request May 25, 2023 that will close this issue
@Goose97
Copy link
Author

Goose97 commented May 25, 2023

Sorry, I almost forgot about this. I've been kinda busy lately.

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 a pull request may close this issue.

2 participants