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

feat(outbound-pg): Reuse connection during request lifecycle #791

Merged
merged 2 commits into from
Sep 28, 2022

Conversation

etehtsea
Copy link
Contributor

@etehtsea etehtsea commented Sep 25, 2022

Also, pg_backend_pid endpoint into outbound-pg example.

Before the fix:

$ curl -i localhost:3000/pg_backend_pid
HTTP/1.1 500 Internal Server Error
content-length: 0
date: Tue, 27 Sep 2022 14:03:50 GMT

thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
  left: `592913`,
  right: `592914`', src/lib.rs:112:5

After the fix:

$ curl -i localhost:3000/pg_backend_pid
HTTP/1.1 200 OK
content-length: 23
date: Tue, 27 Sep 2022 14:07:14 GMT

pg_backend_pid: 595194

Ideally, this fix has to be covered by an integration test rather than
manual testing through examples, but the testing environment has to be
set up first.

Refs: #667.
Signed-off-by: Konstantin Shabanov mail@etehtsea.me

@etehtsea etehtsea force-pushed the enh-pg-out-example branch 3 times, most recently from 4116beb to a1d164e Compare September 27, 2022 14:43
Uncomment write fn and map it to the `/write` endpoint.
Also, add basic README.

Signed-off-by: Konstantin Shabanov <mail@etehtsea.me>
@etehtsea etehtsea changed the title feat(examples): Expand rust-outbound-pg example feat(outbound-pg): Reuse connection during request lifecycle Sep 27, 2022
Also, pg_backend_pid endpoint into outbound-pg example.

Before the fix:

$ curl -i localhost:3000/pg_backend_pid
HTTP/1.1 500 Internal Server Error
content-length: 0
date: Tue, 27 Sep 2022 14:03:50 GMT

thread '<unnamed>' panicked at 'assertion failed: `(left == right)`
  left: `592913`,
  right: `592914`', src/lib.rs:112:5

After the fix:

$ curl -i localhost:3000/pg_backend_pid
HTTP/1.1 200 OK
content-length: 23
date: Tue, 27 Sep 2022 14:07:14 GMT

pg_backend_pid: 595194

Ideally, this fix has to be covered by an integration test rather than
manual testing through examples, but the testing environment has to be
set up first.

Refs: fermyon#667.

Signed-off-by: Konstantin Shabanov <mail@etehtsea.me>
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @etehtsea, looks good to me! Really appreciate the extra info and polish in the sample, and nice to have the connection reuse. Are you still pushing updates or is this okay to merge?

@etehtsea
Copy link
Contributor Author

@itowlson yep, it should be ready to go. The only thing I was thinking about is the difference with redis connections map implementation - https://github.com/fermyon/spin/blob/main/crates/outbound-redis/src/lib.rs#L19-L21

My understanding is that as the state is built per request then it's single-threaded. Am I missing something?

@lann
Copy link
Collaborator

lann commented Sep 28, 2022

My understanding is that as the state is built per request then it's single-threaded. Am I missing something?

The difference with the outbound-redis example is that it is reusing connections between requests. Since this PR only caches within a single request it does not need to lock; individual requests are handled on a single thread.

Edit: We will probably be removing that inter-request caching from outbound-redis. Among other reasons, it has the potential to leak resources in a long-lived spin instance.

@itowlson
Copy link
Contributor

@etehtsea Given Lann's edit to his comment, it looks like the pattern we should be adopting is the one you already have, so I'll merge this as it stands. Thanks for the contribution and for taking the time to cross-check!

@itowlson itowlson merged commit e4245de into fermyon:main Sep 28, 2022
@etehtsea etehtsea deleted the enh-pg-out-example branch September 29, 2022 01:45
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.

3 participants