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

Thread safety #77

Closed
minad opened this issue Jan 3, 2013 · 7 comments
Closed

Thread safety #77

minad opened this issue Jan 3, 2013 · 7 comments

Comments

@minad
Copy link

minad commented Jan 3, 2013

HTTPI doesn't seem to be thread safe because of global state. This is bad :(

Also related to #73

@rubiii
Copy link
Contributor

rubiii commented Jan 3, 2013

could you describe your problem or perhaps even provide code/specs to reproduce it?

@minad
Copy link
Author

minad commented Jan 3, 2013

The adapter object is shared globally. If you use HTTPI from multiple threads it won't work.

@rubiii
Copy link
Contributor

rubiii commented Jan 3, 2013

we still need a spec to verify a potential fix. i think @rogerleite was planning to work on adapters after his holidays.
so a failing spec for this could help redesign the current adapter implementation.

@minad
Copy link
Author

minad commented Jan 3, 2013

This is obvious from the design. Maybe it would be better to redesign the library somehow, to allow persistent connections etc. and make it thread safe.

client = HTTPI.new
client.get
...
client.close

One could still provide convenience methods HTTPI.get etc which have the overhead that they always create a new HTTPI instance each time.

@minad
Copy link
Author

minad commented Jan 6, 2013

I looked again at your code more carefully - no I am not so confused anymore. The adapter isn't shared as it seemed to me before, so no problem. But I still wonder why do you not have a HTTPI::Client class. Then would could also implement persistent connections.

@minad minad closed this as completed Jan 6, 2013
@rubiii
Copy link
Contributor

rubiii commented Jan 6, 2013

because it's simply not that easy to support. see #41

@minad
Copy link
Author

minad commented Jan 6, 2013

Ah ok! Maybe drop the backends which don't support such a design...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants