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

Short circuit WritingTask and await its completion in dispose(async) #44

Merged
merged 2 commits into from
Feb 23, 2024

Conversation

Mpdreamz
Copy link
Member

This to ensure there is no ongoing IO when _streamWriter disposes:

  Error: System.InvalidOperationException : The stream is currently in use by a previous operation on the stream.

    Failed Elastic.OpenTelemetry.Tests.TransactionIdProcessorTests.TransactionId_IsAddedToTags [564 ms]
    Error Message:
     System.InvalidOperationException : The stream is currently in use by a previous operation on the stream.
    Stack Trace:
       at System.IO.StreamWriter.ThrowAsyncIOInProgress()
     at System.IO.StreamWriter.Dispose(Boolean disposing)
     at System.IO.TextWriter.Dispose()
     at Elastic.OpenTelemetry.Diagnostics.LogFileWriter.Dispose() in /home/runner/work/elastic-otel-dotnet/elastic-otel-dotnet/src/Elastic.OpenTelemetry/Diagnostics/LogFileWriter.cs:line 297
     at Elastic.OpenTelemetry.AgentBuilder.Agent.Dispose() in /home/runner/work/elastic-otel-dotnet/elastic-otel-dotnet/src/Elastic.OpenTelemetry/AgentBuilder.cs:line 270
     at Elastic.OpenTelemetry.Tests.TransactionIdProcessorTests.TransactionId_IsAddedToTags() in /home/runner/work/elastic-otel-dotnet/elastic-otel-dotnet/tests/Elastic.OpenTelemetry.Tests/TransactionIdProcessorTests.cs:line 36
     at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
     at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

this was highlighted on our CI test runs:

https://github.com/elastic/elastic-otel-dotnet/actions/runs/8020502607/job/21910481682?pr=39

This to ensure there is no ongoing IO when _streamWriter disposes:

```
  Error: System.InvalidOperationException : The stream is currently in use by a previous operation on the stream.

    Failed Elastic.OpenTelemetry.Tests.TransactionIdProcessorTests.TransactionId_IsAddedToTags [564 ms]
    Error Message:
     System.InvalidOperationException : The stream is currently in use by a previous operation on the stream.
    Stack Trace:
       at System.IO.StreamWriter.ThrowAsyncIOInProgress()
     at System.IO.StreamWriter.Dispose(Boolean disposing)
     at System.IO.TextWriter.Dispose()
     at Elastic.OpenTelemetry.Diagnostics.LogFileWriter.Dispose() in /home/runner/work/elastic-otel-dotnet/elastic-otel-dotnet/src/Elastic.OpenTelemetry/Diagnostics/LogFileWriter.cs:line 297
     at Elastic.OpenTelemetry.AgentBuilder.Agent.Dispose() in /home/runner/work/elastic-otel-dotnet/elastic-otel-dotnet/src/Elastic.OpenTelemetry/AgentBuilder.cs:line 270
     at Elastic.OpenTelemetry.Tests.TransactionIdProcessorTests.TransactionId_IsAddedToTags() in /home/runner/work/elastic-otel-dotnet/elastic-otel-dotnet/tests/Elastic.OpenTelemetry.Tests/TransactionIdProcessorTests.cs:line 36
     at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
     at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
```

this was highlighted on our CI test runs:

https://github.com/elastic/elastic-otel-dotnet/actions/runs/8020502607/job/21910481682?pr=39
Copy link
Contributor

@stevejgordon stevejgordon left a comment

Choose a reason for hiding this comment

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

Good find! LGTM

@Mpdreamz Mpdreamz merged commit 3284bfc into main Feb 23, 2024
3 checks passed
@Mpdreamz Mpdreamz deleted the fix/log-writer-dispose branch February 23, 2024 15:51
@Mpdreamz Mpdreamz added this to the 0.1.0 Alpha milestone Mar 6, 2024
@Mpdreamz Mpdreamz self-assigned this Mar 6, 2024
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