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

Avoid a dependency cycle between Client and Connection classes. #353

Merged
merged 1 commit into from
Jun 10, 2016
Merged

Avoid a dependency cycle between Client and Connection classes. #353

merged 1 commit into from
Jun 10, 2016

Conversation

jhbabon
Copy link
Contributor

@jhbabon jhbabon commented Jun 10, 2016

I found out that sometimes you can get this error when using the gem:

from /.../gems/http-2.0.1/lib/http.rb:8:in  `<top (required)>'
from /.../gems/http-2.0.1/lib/http.rb:8:in  `require'
from /.../gems/http-2.0.1/lib/http/client.rb:7:in  `<top (required)>'
from /.../gems/http-2.0.1/lib/http/client.rb:7:in  `require'
from /.../gems/http-2.0.1/lib/http/connection.rb:4:in  `<top (required)>'
from /.../gems/http-2.0.1/lib/http/connection.rb:4:in  `require'

The problem was that http.rb was requiring http/client.rb that at the same time was requiring http/connection.rb which was requiring http/client.rb again.

I think that in general is not very good to have a cycle like this in the dependency graph of the code, so I decided to remove that dependency by moving the constants KEEP_ALIVE and CLOSE to the Connection class. This way Connection is now independent from Client.

Maybe this constants could go to another module but I couldn't think in a better place.

I found out that sometimes you can get this error when using the gem:

    from /.../gems/http-2.0.1/lib/http.rb:8:in  `<top (required)>'
    from /.../gems/http-2.0.1/lib/http.rb:8:in  `require'
    from /.../gems/http-2.0.1/lib/http/client.rb:7:in  `<top (required)>'
    from /.../gems/http-2.0.1/lib/http/client.rb:7:in  `require'
    from /.../gems/http-2.0.1/lib/http/connection.rb:4:in  `<top (required)>'
    from /.../gems/http-2.0.1/lib/http/connection.rb:4:in  `require'

The problem was that 'http.rb' was requiring 'http/client.rb' that at
the same time was requiring 'http/connection.rb' which was requiring
'http/client.rb' again.

I think that in general is not very good to have a cycle like this in
the dependency graph of the code, so I decided to remove that dependency
by moving the constants KEEP_ALIVE and CLOSE to the Connection class.
This way Connection is now independent from Client.

Maybe this constants could go to another module but I couldn't think in
a better place.
@ixti ixti merged commit 61d8b5c into httprb:master Jun 10, 2016
@ixti
Copy link
Member

ixti commented Jun 10, 2016

👍 Thank you!

@jhbabon jhbabon deleted the avoid-dependency-cycle branch June 10, 2016 16:20
@jhbabon
Copy link
Contributor Author

jhbabon commented Jun 10, 2016

That was fast. Thanks 😄

@jhbabon
Copy link
Contributor Author

jhbabon commented Jun 22, 2016

@ixti Hi!
Sorry to bother you, but do you know if there is any plan to release a new version of the gem with this fix?

@ixti
Copy link
Member

ixti commented Jun 24, 2016

Sorry for delay. v2.0.2 with this patch just got released. Thanks for your contribution once again!

@jhbabon
Copy link
Contributor Author

jhbabon commented Jun 27, 2016

@ixti thank you for your work!

jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Oct 18, 2016
## 2.0.3 (2016-08-03)

* [#365](httprb/http#365)
  Add `HTTP::Response#content_length`
  ([@janko-m])

* [#335](httprb/http#335),
  [#360](httprb/http#360)
  Set `Content-Length: 0` header for `nil` bodies.
  ([@britishtea])


## 2.0.2 (2016-06-24)

* [#353](httprb/http#353)
  Avoid a dependency cycle between Client and Connection classes.
  ([@jhbabon])


## 2.0.1 (2016-05-12)

* [#341](httprb/http#341)
  Refactor some string manipulations so they are more performant
  (up to 3-4x faster) and more concise.
  ([@tonyta])

* [#339](httprb/http#341)
  Always use byte methods when writing/slicing the write buffer.
  ([@zanker])


## 2.0.0 (2016-04-23)

* [#333](httprb/http#333)
  Fix HTTPS request headline when sent via proxy.
  ([@Connorhd])

* [#331](httprb/http#331)
  Add `#informational?`, `#success?`, `#redirect?`, `#client_error?` and
  `#server_error?` helpers to `Response::Status`.
  ([@mwitek])

* [#330](httprb/http#330)
  Support custom CONNECT headers (request/response) during HTTPS proxy requests.
  ([@smudge])

* [#319](httprb/http#319)
  Drop Ruby 1.9.x support.
  ([@ixti])


## 1.0.4 (2016-03-19)

* [#320](httprb/http#320)
  Fix timeout regression.
  ([@tarcieri])


## 1.0.3 (2016-03-16)

* [#314](httprb/http#314)
  Validate charset before forcing encoding.
  ([@kylekyle])

* [#318](httprb/http#318)
  Remove redundant string allocations upon header names normalization.
  ([@ixti])
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