Skip to content
This repository has been archived by the owner on Jun 1, 2024. It is now read-only.
This repository has been archived by the owner on Jun 1, 2024. It is now read-only.

Why is elasticsearch sink using sync version of HttpConnection.Request<TResponse> ? #346

Closed
HassanHashemi opened this issue Jul 22, 2020 · 1 comment

Comments

@HassanHashemi
Copy link
Contributor

HassanHashemi commented Jul 22, 2020

So, I get following call stack in my logs

  at Elasticsearch.Net.HttpConnection.Request[TResponse](RequestData requestData)
  at Elasticsearch.Net.RequestPipeline.CallElasticsearch[TResponse](RequestData requestData)
  at Elasticsearch.Net.Transport`1.Request[TResponse](HttpMethod method, String path, PostData data, IRequestParameters requestParameters)
  at Serilog.Sinks.Elasticsearch.ElasticsearchSink.EmitBatchChecked[T](IEnumerable`1 events)
  at Serilog.Sinks.Elasticsearch.ElasticsearchSink.EmitBatch(IEnumerable`1 events)
  at Serilog.Sinks.PeriodicBatching.PeriodicBatchingSink.EmitBatchAsync(IEnumerable`1 events)

In the end HttpConnection.Request is called synchronously, should not it be RequestAsync?

https://github.com/elastic/elasticsearch-net/blob/b40781873a1d79a582944851c43189db0fc2aea2/src/Elasticsearch.Net/Connection/HttpConnection.cs#L78

It should be possible to call async version in a void method, as long as we handle exceptions carefully.

I am OK with sending a PR if this change is fine?
@mivano

@mivano
Copy link
Contributor

mivano commented Jul 28, 2020

Hmm that implementation is pretty old and since we do IO operations, we could easily use the async parts. Unfortunately the EmitBatch is not async, but there is an EmitBatchAsync in the PeriodicBatchingSink base class. Might be nice to switch over to that implementation? I certainly welcome a PR!

@HassanHashemi HassanHashemi changed the title Why is elasticsearch sink using sync version of HttpContext.Request<TResponse> ? Why is elasticsearch sink using sync version of HttpConnection.Request<TResponse> ? Jul 29, 2020
@HassanHashemi HassanHashemi mentioned this issue Jul 29, 2020
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants