risor-io / risor

Fast and flexible scripting for Go developers and DevOps.
https://risor.io
Apache License 2.0
610 stars 26 forks source link

Simplify limit tracking; make limits thread-safe #201

Closed myzie closed 6 months ago

myzie commented 6 months ago

The limits feature is not much used and was not implemented consistently across different built-ins. This meant it was adding complexity to the codebase without adding much value. Due to gaps in where limits were applied, a developer using Risor may have a false sense of security from it.

Given all that, I'm removing limit related behavior from much of the codebase. The limits mechanism is still available, and can be opted into by writing your own built-ins that leverage it. I did also keep the limits hookup for outgoing HTTP requests specifically, since that was fairly well defined and works.

This also updates the limits implementation to be threadsafe, which is needed now that Risor supports goroutines.

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 53.33333% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 31.52%. Comparing base (297643d) to head (54aaa70).

Files Patch % Lines
limits/standard_limits.go 50.00% 4 Missing and 1 partial :warning:
modules/http/request.go 66.66% 2 Missing and 2 partials :warning:
builtins/builtins.go 0.00% 2 Missing :warning:
object/file.go 0.00% 1 Missing :warning:
os/localfs/localfs.go 0.00% 1 Missing :warning:
os/virtual.go 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #201 +/- ## ========================================== + Coverage 31.35% 31.52% +0.16% ========================================== Files 113 113 Lines 14529 14461 -68 ========================================== + Hits 4556 4559 +3 + Misses 9398 9331 -67 + Partials 575 571 -4 ```

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

myzie commented 6 months ago

fyi @edhemphill @luisdavim @applejag. I'm guessing you won't object to this simplification, but let me know if otherwise.

luisdavim commented 6 months ago

Thanks for letting us know.