search

hummingbird-project / hummingbird

Lightweight, flexible HTTP server framework written in Swift
Apache License 2.0
1.16k stars 53 forks source link

Use Foundation.ProcessInfo for environment parsing #573

Closed GNMoseke closed 1 week ago

GNMoseke commented 2 weeks ago

Resolves #571

Joannis commented 2 weeks ago

My only (slight) concern is that I believe this wasn't thread safe in 5.9

I don't know if it's still the case - or if it matters much at all. Would need to verify that

GNMoseke commented 2 weeks ago

Yeah I saw that in the issue - I can verify in 5.9, will draft this guy for now.

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 83.68%. Comparing base (e78cde7) to head (94ae83f). Report is 144 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #573 +/- ## ========================================== - Coverage 84.86% 83.68% -1.19% ========================================== Files 98 99 +1 Lines 5320 4406 -914 ========================================== - Hits 4515 3687 -828 + Misses 805 719 -86 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

GNMoseke commented 2 weeks ago

At least as of 5.9.1, the implementation of ProcessInfo.processInfo.environment is the same as what this PR replaces - so there should be no difference for 5.9

The underlying access is indeed a char** as @adam-fowler commented in the original issue - I am sorry to say that my low-level cpp knowledge isn't super strong, so this appears thread safe to me, but take that with a grain of salt

GNMoseke commented 1 week ago

Alright fellas sorry it took me so long to get back to this - as mentioned in #574 there's some thread safety issues here.

I think what was happening is that ProcessInfo.processInfo, as a static variable, along with Environment.getEnvironment as a static method were doing something funky when all tests were run in a single process (from a normal swift test invocation). About 1/5 times there I could reproduce the failure, including on a 6.0.1 toolchain.

I could not reproduce the failure running swift test --parallel, which is what leads me to believe that this only pops up in the case where all the tests are being started from the same process.

As a band-aid I renamed the env var that testSetForAllEnvironments uses, which feels like a bit of a hacky solution but does resolve the test failure.

All of the above said, I am very new to the codebase here and could absolutely be misunderstanding what's happening, so if any of that^ is untrue, please call me on it.

TL;DR I don't think this is actually an issue in a real running hummingbird instance since the server is started as a single process and the environment is being properly mutated - just a race in tests.

adam-fowler commented 1 week ago

As a band-aid I renamed the env var that testSetForAllEnvironments uses, which feels like a bit of a hacky solution but does resolve the test failure.

I think this is fine. The tests should be treated as independent and not affect each other.