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

Threading: do not memoize/reuse Patron session #796

Merged
merged 1 commit into from
May 10, 2018
Merged

Threading: do not memoize/reuse Patron session #796

merged 1 commit into from
May 10, 2018

Conversation

julik
Copy link
Contributor

@julik julik commented May 7, 2018

This reverts changes from b0b1e38 and the Session object is going to be configured anew for each request.

The rationale/reason is that Patron session objects are not thread-safe by default, but are rather meant to be used thread-locally or pooled. Contrary to what one would believe, retaining a Patron session will not lead to the libCURL handle being reused across requests to achieve keepalive. Patron is architected in such a way that all resources associated to a specific request get initialized and allocated right when the request begins, and get freed/deallocated once the response has been read out or an abort has been raised. Therefore the only memory saving achieved from preserving a Patron session is the Ruby object for the Session itself.

This retaining however will lead to unpleasant situations with threads, since the same session might end up being reused by different threads. For example, when configuring Faraday like so:

Faraday.default_connection = Faraday.new {|c|
  c.adapter :patron
}

there will be effectively one Session object reused across threads. Since Patron unlocks the GIL during request/response, this will lead to threads receiving response bodies for other threads and other unpleasant occurrences.

Also passes the block in create_connection in integration tests as the adapter configuration block.

Todos

  • Tests

This reverts changes from b0b1e38 and the Session
object is going to be configured anew for each request.

The rationale/reason is that Patron session objects are
not thread-safe by default, but are rather meant to be
used thread-locally **or** pooled. Contrary to what
one would believe, retaining a Patron session will not
lead to the libCURL handle being reused across requests
to achieve keepalive. Patron is architected in such a way
that all resources associated to a specific request
get initialized and allocated right when the request
begins, and get freed/deallocated once the response
has been read out or an abort has been raised. Therefore
the only memory saving achieved from preserving a Patron
session is the Ruby object for the Session itself.

This retaining however will lead to unpleasant situations
with threads, since the same session might end up being
reused by different threads. For example, when configuring
Faraday like so:

    Faraday.default_connection = Faraday.new {|c|
      c.adapter :patron
    }

there will be effectively _one_ Session object reused across
threads. Since Patron unlocks the GIL during request/response,
this will lead to threads receiving response bodies for
other threads and other unpleasant occurrences.

Also passes the block in create_connection in integration
tests as the adapter configuration block.
@iMacTia iMacTia self-requested a review May 8, 2018 13:57
@iMacTia
Copy link
Member

iMacTia commented May 10, 2018

Thanks @julik for fixing this.
You're the Patron guru so I'll trust you blindly on this one 😄.

@iMacTia iMacTia merged commit ac66113 into lostisland:master May 10, 2018
@derSascha derSascha mentioned this pull request Jul 13, 2019
2 tasks
@julik
Copy link
Contributor Author

julik commented Jul 14, 2019

For the record: my statement in

Contrary to what one would believe, retaining a Patron session will not lead to the libCURL handle being reused across requests to achieve keepalive.

is not correct. Patron will achieve keepalive if the server supports it and you perform subsequent calls to the same server/port/proto.

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