Skip to content

Add support for 'concurrent_http' requests in Binance feeds#580

Merged
bmoscon merged 5 commits intobmoscon:masterfrom
jinusean:binance-concurrent-http
Jul 29, 2021
Merged

Add support for 'concurrent_http' requests in Binance feeds#580
bmoscon merged 5 commits intobmoscon:masterfrom
jinusean:binance-concurrent-http

Conversation

@jinusean
Copy link
Copy Markdown

  • Allow concurrent_http requests in Binance feeds: L2_BOOK, and OPEN_INTEREST
  • Created HTTPConcurrentPoll connection type for concurrent polling.
  • Updated examples/demo_proxy.py -> examples/demo_concurrent_proxy.py

Please see output in examples/demo_concurrent_proxy.py to see performance improvements (no proxy necessary).

  • - Tested
  • - Changelog updated
  • - Tests run and pass
  • - Flake8 run and all errors/warnings resolved
  • - Contributors file updated (optional)

Copy link
Copy Markdown
Owner

@bmoscon bmoscon left a comment

Choose a reason for hiding this comment

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

see comments

try:
while True:
data = await self._read_address(address, header)
await asyncio.gather(self._queue.put(data), asyncio.sleep(self.sleep))
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

im not sure why you are using gather here. You have already spawned a new task to run _poll_address, why create more tasks to put the data on the queue and then wait? cant you just await the queue.put and then await the sleep?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The rationale was to keep the callback time more uniform to the user specified sleep time. For example, if sleep was set to 1 second, and a request takes 0.2 seconds, each callback would have a latency of 1.2 seconds. Whereas by using gather the latency would more closely resemble specified sleep time.

Please let me know if you're not a fan of this approach and I will change the implementation.

@bmoscon bmoscon merged commit d4d8918 into bmoscon:master Jul 29, 2021
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