ThreeMammals / Ocelot

.NET API Gateway

https://www.nuget.org/packages/Ocelot

MIT License

8.32k stars 1.63k forks source link

#1700 Create custom Ocelot config file when instantiating steps during parallel execution #1703

Closed ggnaegi closed 12 months ago

ggnaegi commented 12 months ago

Fixes #1700

1700

When instantiating steps, creating a custom ocelot config file name. Finally deleting the newly created config file when disposing the Step instance.

Proposed Changes

Since, by design, a new instance of Step is instantiated for each fact/theory, we need to ensure that the corresponding fact/theory only uses its own ocelot configuration file, without interfering with the configuration files of other facts and theories.

raman-m commented 12 months ago

Wow! Cool! Let's make 10 PR builds of 10 commits being added to this PR, to get some statistics on stability of this fix.

Just commit a small change of Ocelot core, not tests, and the 2nd commit should be revert commit of previous one. So, there will be 5 commits and 5 revert-commits.

Good plan?

ggnaegi commented 12 months ago

@raman-m no way, that's really funny ;-) 6 out of 10... I can't reproduce that on my machine

ggnaegi commented 12 months ago

@raman-m Ok, a bit better now, 9 out of 11...

ggnaegi commented 12 months ago

@raman-m why did you remove xunit.runner.json?

raman-m commented 12 months ago

I've reverted commit 9fac778 Sorry!

It seems your solution doesn't work for some reason... That's strange... It should...

Have a rest today! 😉 We will continue tomorrow, this week...

raman-m commented 12 months ago

@ggnaegi commented

why did you remove xunit.runner.json?

Because you've disabled parallel execution for all tests by default.

ggnaegi commented 12 months ago

@ggnaegi commented

why did you remove xunit.runner.json?

Because you've disabled parallel execution for all tests by default.

Yeah, I tried...

ggnaegi commented 12 months ago

@raman-m 11 out of 12. It's not that bad...

ggnaegi commented 12 months ago

@raman-m we might have a solution...

raman-m commented 12 months ago

It seems this fix works! 👍 What was the root cause of failing? I don't think it was random port finder... 🤣

raman-m commented 12 months ago

Do you confirm that we can still use parallelization of all tests?

ggnaegi commented 12 months ago

Do you confirm that we can still use parallelization of all tests?

@raman-m Yes, we still have an issue with Open_circuit_should_not_effect_different_route though... That's the test failing at the minute. I can reproduce that on my computer, it fails after 20 minutes.

raman-m commented 12 months ago

Open_circuit_should_not_effect_different_route

Forget about this test now. It is out of the scope of linked bug-issue. 🆗 I'm going to review...

ggnaegi commented 12 months ago

Open_circuit_should_not_effect_different_route

Forget about this test now. It is out of the scope of linked bug-issue. 🆗 I'm going to revi

It seems this fix works! 👍 What was the root cause of failing? I don't think it was random port finder... 🤣

@raman-m No it wasn't, but the random port finder code doesn't seem to be thread safe, since the concurrentbag would allow identical values to be stored and the method can be accessed by several threads concurrently

ggnaegi commented 12 months ago

@raman-m imho ok to merge

raman-m commented 12 months ago

Revert "commit #h5" 6e4b8a4

@ggnaegi #1706 FYI

raman-m commented 12 months ago

@ggnaegi Congrats! We did that! Thank you very much for your efforts during this Sunday's evening!