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

Reuse Patron session with mutex #1002

Conversation

derSascha
Copy link

Reuse Patron session to support keep-alive/connection reuse and make
them thread-safe using a mutex.

Description

This reverts the changes from PR #796 and reuse the patron session for keep-alive/connection reuse. To be thread safe, this adds a mutex around the patron session. See persisted connections and Threads in the patron documentation for more details.

Fixes #1001

Todos

  • Tests
  • Documentation

Reuse Patron session to support keep-alive/connection reuse and make
them thread-safe using a mutex.

fixes #1001
@derSascha derSascha force-pushed the patron-thread-safe-session-reuse branch from 68b8949 to 67477d6 Compare July 13, 2019 16:06
Copy link
Member

@iMacTia iMacTia left a comment

Choose a reason for hiding this comment

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

The PR itself makes sense, although the new test added doesn't really test anything (if I understand the PR correctly). You can also ignore the CodeClimate issues as those were not addressed with call to begin with.

However, even if the test is fixed based on my comment, I'd still like to get to a resolution on #1001 before progressing with this.

conn = Faraday.new do |f|
f.adapter :patron do |session|
if last_session
expect(session).to eq last_session
Copy link
Member

Choose a reason for hiding this comment

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

Given @config_block&.call(session) will be called only once due to the cache, I wouldn't expect this expect to ever be executed.

Instead, you could use a variable to check that this block is only called once:

    config_calls = 0
    conn = Faraday.new do |f|
      f.adapter :patron do |session|
        config_calls += 1
      end
    end

and then after the two conn.get('http://example.com/') you can

    expect(config_calls).to eq(1)

@iMacTia
Copy link
Member

iMacTia commented Jul 15, 2019

@derSascha I really appreciate you taking a stab at this, however as the discussion in #1001 shows, this is not the solution we'd like to go with, so I'm closing this PR.

I'll do my best to help agreeing and implementing a solution.

@iMacTia iMacTia closed this Jul 15, 2019
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.

Patron adapter does not support keep-alive
2 participants