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

Make the network request asynchronous #2

Open
adityar7 opened this issue Dec 10, 2013 · 4 comments · May be fixed by #19
Open

Make the network request asynchronous #2

adityar7 opened this issue Dec 10, 2013 · 4 comments · May be fixed by #19

Comments

@adityar7
Copy link

Blocking the server code is unrealistic in most cases. The network request should be done asynchronously.

Would you consider merging something like the following commit that uses a separate thread?

adityar7@770d199

@patrys
Copy link
Contributor

patrys commented Dec 10, 2013

Wouldn't it be simpler to have a thread call report(…) instead? For example in saleor project I am planning to introduce a thread pool to do just that.

@adityar7
Copy link
Author

Well, I don't know if it'll be simpler since everyone who uses the app will end up having to write the same code, since this really needs to be non-blocking for almost every use case. So it may as well be pushed down to the app.

It could be made optional via a parameter. I didn't add that yet to get your comments on the approach in case you'd want to merge something like this.

@patrys
Copy link
Contributor

patrys commented Dec 10, 2013

In either case it would be much simpler to introduce thread pooling at the report(…) level (as opposed to creating a new thread for each call). For example by introducing a thread pool object that exposes a .report(…) method with an identical signature.

I am not against merging that to the library itself, I'd just prefer to keep the extra complexity out of the existing reporting mechanism.

@adityar7
Copy link
Author

Sure, a thread pool will be better indeed. I'll try to update my patch some time soon.

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 a pull request may close this issue.

2 participants