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

Exception caught in global error handler, exception message: Object reference not set to an instance of an object. #1608

Open
klaudiusz-czapla opened this issue Oct 6, 2022 · 16 comments
Assignees
Labels
bug Identified as a potential bug

Comments

@klaudiusz-czapla
Copy link

klaudiusz-czapla commented Oct 6, 2022

Expected Behavior

I'm expecting to get error details coming from ocelot.json file validation in case of invalid configuration being provided.

Actual Behavior

I had scenario where Errors collection coming from validation process (ocelot.json) is discarded and then I'm getting NullReferenceException instead which is hiding the real problem.

Exception caught in global error handler, exception message: Object reference not set to an instance of an object., exception stack:    
at Ocelot.Errors.Middleware.ExceptionHandlerMiddleware.TrySetGlobalRequestId(HttpContext httpContext, IInternalConfiguration configuration)\n   
at Ocelot.Errors.Middleware.ExceptionHandlerMiddleware.Invoke(HttpContext httpContext) 

Steps to Reproduce the Problem

You have to create ocelot.json file where one of the routes contains something like this:

"DownstreamPathTemplate": "{everything}"

During debugging Ocelot I got at some point Errors collection with message: "Downstream Path Template {everything} doesnt start with forward slash"
It was in method Ocelot.Middleware.OcelotMiddlewareExtensions.CreateConfiguration'IApplicationBuilder -> fileConfig.OnChange callback

Ocelot.dll!Ocelot.Configuration.Repository.InMemoryInternalConfigurationRepository.AddOrReplace(Ocelot.Configuration.IInternalConfiguration internalConfiguration) Line 27	C#
>	Ocelot.dll!Ocelot.Middleware.OcelotMiddlewareExtensions.CreateConfiguration.AnonymousMethod__0(Ocelot.Configuration.File.FileConfiguration config) Line 98	C#
 	Microsoft.Extensions.Options.dll!Microsoft.Extensions.Options.OptionsMonitorExtensions.OnChange.AnonymousMethod__0(System.__Canon o, string _)	Unknown

nevertheless somehow this mesage was never logged anywhere, it was discarded and finally I got NullReferenceException instead

{Ocelot.Responses.ErrorResponse<Ocelot.Configuration.IInternalConfiguration>}
-> Data -> null in my case
-> Errors -> Downstream Path Template {everything} doesnt start with forward slash
-> IsError -> true

In line

internalConfigRepo.AddOrReplace(newInternalConfig.Data);

error should be checked before accessing .Data property

Specifications

  • Version: 17.0.0, 17.0.1
  • Platform: Windows/Linux
  • Subsystem: -
@ntruongvux
Copy link

Maybe, you configured duplication the endpoint about downstream/upstream url

@raman-m
Copy link
Member

raman-m commented Jul 3, 2023

Klaudiusz, Klaudiusz, Klaudiusz... 😄

Hi Klaudiusz!
It is absolutely normal that if you use invalid ocelot.json file then Ocelot generates exceptions. 😸
I would recommend you to check and validate your ocelot.json file before any deployments!


You have to create ocelot.json file where one of the routes contains sth like this:
"DownstreamPathTemplate": "{everything}"

This is wrong template! You must add slash sign before {everything}
According to the Routing feature docs, valid template is: /{everything}


During debugging Ocelot I got at some point Errors collection with message: "Downstream Path Template {everything} doesnt start with forward slash"

Bingo! 🤣


Have a great summer, Klaudiusz! 🌞

@raman-m raman-m added bug Identified as a potential bug invalid Not actually an issue wontfix No plan to include this issue in the Ocelot core functionality. labels Jul 3, 2023
@raman-m
Copy link
Member

raman-m commented Jul 3, 2023

Specifications

  • Version: 17.0.0, 17.0.1
  • Platform: Windows/Linux
  • Subsystem: -

Please, upgrade your solution to v19 (.NET 7 release)

@raman-m raman-m closed this as not planned Won't fix, can't repro, duplicate, stale Jul 3, 2023
@klaudiusz-czapla
Copy link
Author

klaudiusz-czapla commented Jul 3, 2023

@raman-m Error message which I've posted was extracted by myself during debugging. In the runtime it was hidden from the user. Btw NullReferenceException should never be thrown by the application. It should be handled somehow. As I said - original error message was hidden. Without debugging Ocelot code I would never be able to get to know what was wrong and misconfigured :)

@raman-m raman-m reopened this Jul 3, 2023
@raman-m
Copy link
Member

raman-m commented Jul 3, 2023

Aha, I see. Sorry!
I've reopened the bug.
What's your fix?
Are you going to create a PR?

@raman-m
Copy link
Member

raman-m commented Jul 6, 2023

I would love to review your PR and/or discuss your idea to fix this error message hiding problem.
And show me a line in source code which generates NullReferenceException please!

@ks1990cn
Copy link
Contributor

ks1990cn commented Dec 1, 2023

@klaudiusz-czapla Hi,
I am unable to replicate NullReferenceException, when configuration is bad. I can see following validation code which doesn't allow to start application even.

image

My Route in Ocelot.json file is, here I removed first slash of downstream path.

 "Routes": [
   {
     "DownstreamHostAndPorts": [
       {
         "Host": "localhost",
         "Port": 5022
       },
       {
         "Host": "localhost",
         "Port": 5085
       }
     ],
     "ServiceName": "ConsulDemo",
     "LoadBalancerOptions": {
       "Type": "LeastConnection"
     },
     "UpstreamPathTemplate": "/hostname",
     "UpstreamHttpMethod": [ "GET" ],
     "DownstreamPathTemplate": "WeatherForecast/hostname",
     "DownstreamScheme": "http",
     "DangerousAcceptAnyServerCertificateValidator": true


   }
 ]

@raman-m Not sure what he was trying to explain last year, but on latest version I am unable to see any issue.
@klaudiusz-czapla Feel free to add any point here if I missed anything, else @raman-m we can close this. Thanks!

@raman-m
Copy link
Member

raman-m commented Dec 4, 2023

These lines are under discussion: lines L92-L96
Correct, @klaudiusz-czapla ?

You've shown concrete line of code which fails in your user scenario.
But what's the problem to create a PR? Is it so hard for you?

Regarding NullReferenceException ... Yeap, it seems callback processes null newInternalConfig variable. The fix should be pretty simple. But the root cause is unknown because this callback calls IInternalConfigurationCreator service which has a lot of interconnected injected services in implementation (FileInternalConfigurationCreator.Create)... So, it is hard to say what's happen inside and why var newInternalConfig is null...

@raman-m
Copy link
Member

raman-m commented Dec 4, 2023

@ks1990cn commented on Dec 1

No, he mentioned these lines: lines L92-L96 which are below starting the line 92...

@raman-m raman-m removed invalid Not actually an issue wontfix No plan to include this issue in the Ocelot core functionality. labels Dec 4, 2023
@ks1990cn
Copy link
Contributor

ks1990cn commented Dec 4, 2023

@raman-m how he got an error? I see code is safe

@raman-m
Copy link
Member

raman-m commented Dec 4, 2023

@ks1990cn
It seems the problem is inside the async anonymous callback: fileConfig.OnChange...


The quote from the description:

In line

internalConfigRepo.AddOrReplace(newInternalConfig.Data);

error should be checked before accessing .Data property

@ks1990cn
Copy link
Contributor

ks1990cn commented Dec 4, 2023

Okay, but code stops running before that at line 85.

image

@ks1990cn
Copy link
Contributor

ks1990cn commented Dec 4, 2023

This piece of code never executed in case of bad configuration... @raman-m , still I want to understand how he reached there..?

@ks1990cn
Copy link
Contributor

ks1990cn commented Dec 4, 2023

do someone added this code on latest version?

@raman-m
Copy link
Member

raman-m commented Dec 5, 2023

@ks1990cn

do someone added this code on latest version?

Probably Yes...


Steps to Reproduce the Problem

You have to create ocelot.json file where one of the routes contains something like this:

"DownstreamPathTemplate": "{everything}"

Tanmay, his case is invalid... He was kidding in this issue, and request to fix quite wrong user case.
Here is my very first reply in the thread
So, he replied with short answer that he don't like situation when internal exception was swallowed by Ocelot core and he wants this message in upper contexts. But he pointed to the code which generates this exception.
We can fix this line of code...


Tanmay, once again...
His user scenario is about OnChange callback... Seems you've reproduced different case...
Author's concern is about NullReferenceException in the callback!
We don't have Steps to Repro!!!! Author is silent since July 3... more than 5+ months

We're discussing something quite specific...
Let give him 1 month to open a PR with the fix which is tiny...
If he will be silent we just add that junior style PR to fix line 95, that's it! 🆗?

@raman-m
Copy link
Member

raman-m commented Dec 5, 2023

@klaudiusz-czapla

The title: Exception caught in global error handler

Exactly! Didn't you know the fact that Ocelot has global error handler which processes all exceptions?
Bingo! 💥 You know the fact now!


We've discussed the case. You pointed to the problematic line. The fix should be tiny...
Why not to create a PR?

You have one month till January 5, 2024 to create a PR

...otherwise you won't be an author of the fix, or we will close the issue at all.

CC: klaudiuszc@interia.pl

P.S.

And please fork Ocelot repo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identified as a potential bug
Projects
None yet
Development

No branches or pull requests

4 participants