MindscapeHQ / raygun4net

Raygun provider for .NET
https://raygun.com
MIT License
126 stars 93 forks source link

HTTP request context is lost after building first RaygunRequestMessage #421

Closed Zero3 closed 8 months ago

Zero3 commented 4 years ago

From the source:

    private RaygunRequestMessage BuildRequestMessage()
    {
      var message = _currentWebRequest.Value != null ? RaygunWebApiRequestMessageBuilder.Build(_currentWebRequest.Value, _requestMessageOptions) : null;
      _currentWebRequest.Value = null;
      return message;
    }

This means that every time a RaygunRequestMessage is built by Send() or SendInBackground(), the current HTTP request context is lost when this code sets _currentWebRequest to null for the current thread.

The implication is that each subsequent error sent by a RaygunWebApiClient instance won't contain all the information that normally is extracted from the HTTP request context.

Perhaps a RaygunWebApiClient is only intended to be used to send one error? If so, that is not very clear from the code or documentation, and also kind of contradicts README.md (highlighting by me):

Sometimes when setting up Raygun to send exceptions automatically, you may need to provide a custom RaygunWebApiClient instance in order to use some of the optional feature described below. To do this, use the static RaygunWebApiClient.Attach method overload that takes a function. Within this function, return a new (or previously created) RaygunWebApiClient instance.

knumat commented 4 years ago

This bug still exists. https://github.com/MindscapeHQ/raygun4net/blob/fefc40dc0e5d40691b968c6fa69ab60184457293/Mindscape.Raygun4Net.AspNetCore/RaygunClient.cs#L254

Additionally, this bug also means that Message.Details.Response is never populated, since BuildResponseMessage() is called after BuildRequestMessage() sets _currentHttpContext.Value to null.

What do we need to get this fixed? Should I submit a pull request?

mduncan26 commented 4 years ago

Hi @knumat

Yes, we are open to pull requests. Otherwise I will raise this issue with the team and we will look to create an internal ticket to address this issue. I am unable to give an accurate time in which this issue will be addressed. However as this is a bug within the provider we will look to prioritise it higher. If you wish to submit a PR for this issue that will help us reduce the time it takes to get an official release out. We will update this thread if there is any change to the progress of addressing this issue.

Thank you, Mitchell.

knumat commented 4 years ago

As best I can tell, this bug appears to have been added by the following commit. https://github.com/MindscapeHQ/raygun4net/commit/71940843fc8c4c985cd8d7be1b91df8bdfa87aed#diff-9e319763ead81dded08bb3933104e9620f1d2b94ab7c72cfa2f731650d88b2ecR377

Then it seems to have been carried along when creating the ASP.NET Core version: https://github.com/MindscapeHQ/raygun4net/commit/9b3bf37fc6a30c30afbfa96dcc5d9d4b8605b928#diff-edbe3bb3bac5e2263426f544f90c36a13200e140f76a0653d2ab60b75ad5e7baR240

I don't see any mention of why this might have been necessary. The only reason I can think of would be to avoid a memory leak if the RaygunClient was long-lived. However, my understanding that a new RaygunClient would be created for each request. If that is correct, I can submit a pull request to fix this.

phillip-haydon commented 8 months ago

This should be resolved in v10.0.0.