Closed NachoEchevarria closed 4 days ago
datadog-ddstaging[bot]
commented
1 week ago Branch report: nacho/ObjectDisposedException
Commit report: 795a27a
Test service: dd-trace-dotnet
:white_check_mark: 0 Failed, 342810 Passed, 1620 Skipped, 14h 51m 8.12s Total Time
andrewlock
commented
1 week ago Execution-time results for samples comparing the following branches/commits:
Execution-time benchmarks measure the whole time it takes to execute a program. And are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are shown in red. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard.
Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph).
gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5732) - mean (74ms) : 65, 84
. : milestone, 74,
master - mean (74ms) : 65, 83
. : milestone, 74,
section CallTarget+Inlining+NGEN
This PR (5732) - mean (990ms) : 969, 1011
. : milestone, 990,
master - mean (991ms) : 973, 1009
. : milestone, 991,
gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5732) - mean (109ms) : 106, 112
. : milestone, 109,
master - mean (110ms) : 107, 113
. : milestone, 110,
section CallTarget+Inlining+NGEN
This PR (5732) - mean (702ms) : 681, 724
. : milestone, 702,
master - mean (698ms) : 675, 722
. : milestone, 698,
gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5732) - mean (93ms) : 90, 96
. : milestone, 93,
master - mean (93ms) : 90, 96
. : milestone, 93,
section CallTarget+Inlining+NGEN
This PR (5732) - mean (653ms) : 632, 674
. : milestone, 653,
master - mean (657ms) : 630, 683
. : milestone, 657,
gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.6.2)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5732) - mean (194ms) : 190, 198
. : milestone, 194,
master - mean (192ms) : 188, 196
. : milestone, 192,
section CallTarget+Inlining+NGEN
This PR (5732) - mean (1,096ms) : 1071, 1120
. : milestone, 1096,
master - mean (1,081ms) : 1059, 1103
. : milestone, 1081,
gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5732) - mean (279ms) : 273, 286
. : milestone, 279,
master - mean (276ms) : 271, 281
. : milestone, 276,
section CallTarget+Inlining+NGEN
This PR (5732) - mean (893ms) : 872, 913
. : milestone, 893,
master - mean (874ms) : 849, 899
. : milestone, 874,
gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat X
axisFormat %s
todayMarker off
section Baseline
This PR (5732) - mean (269ms) : 262, 277
. : milestone, 269,
master - mean (269ms) : 263, 275
. : milestone, 269,
section CallTarget+Inlining+NGEN
This PR (5732) - mean (862ms) : 828, 896
. : milestone, 862,
master - mean (867ms) : 839, 894
. : milestone, 867,
Throughput results for AspNetCoreSimpleController comparing the following branches/commits:
Cases where throughput results for the PR are worse than latest master (5% drop or greater), results are shown in red.
Note that these results are based on a single point-in-time result for each branch. For full results, see one of the many, many dashboards!
gantt
title Throughput Linux x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5732) (12.019M) : 0, 12019258
master (11.753M) : 0, 11753412
benchmarks/2.9.0 (11.542M) : 0, 11542126
section Automatic
This PR (5732) (8.136M) : 0, 8136236
master (7.853M) : 0, 7853279
benchmarks/2.9.0 (8.263M) : 0, 8262905
section Trace stats
master (8.259M) : 0, 8258966
section Manual
This PR (5732) (10.315M) : 0, 10315281
master (9.899M) : 0, 9899401
section Manual + Automatic
This PR (5732) (7.692M) : 0, 7691814
master (7.433M) : 0, 7432897
section Version Conflict
master (6.731M) : 0, 6730574
gantt
title Throughput Linux arm64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5732) (9.650M) : 0, 9649980
master (9.574M) : 0, 9574121
benchmarks/2.9.0 (9.596M) : 0, 9596140
section Automatic
This PR (5732) (6.636M) : 0, 6636137
master (6.671M) : 0, 6671192
section Trace stats
master (6.990M) : 0, 6990244
section Manual
This PR (5732) (8.268M) : 0, 8268402
master (8.307M) : 0, 8306500
section Manual + Automatic
This PR (5732) (6.215M) : 0, 6214661
master (6.251M) : 0, 6250704
section Version Conflict
master (5.762M) : 0, 5761603
gantt
title Throughput Windows x64 (Total requests)
dateFormat X
axisFormat %s
section Baseline
This PR (5732) (9.949M) : 0, 9949075
master (10.202M) : 0, 10201759
benchmarks/2.9.0 (10.213M) : 0, 10213239
section Automatic
This PR (5732) (6.992M) : 0, 6992121
master (7.212M) : 0, 7212394
benchmarks/2.9.0 (7.482M) : 0, 7482023
section Trace stats
master (7.549M) : 0, 7549360
section Manual
This PR (5732) (8.622M) : 0, 8621515
master (9.013M) : 0, 9012822
section Manual + Automatic
This PR (5732) (6.706M) : 0, 6705589
master (7.018M) : 0, 7017920
section Version Conflict
master (6.365M) : 0, 6365432
Benchmarks for #5732 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored.
andrewlock
commented
1 week ago Benchmarks for #5732 compared to master:
The following thresholds were used for comparing the benchmark speeds:
Allocation changes below 0.5% are ignored.
robertpi
commented
1 week ago Under such circumstances, we should capture the exception and return null, same as we do in that method when the context is null.
So this method only occurs if there's no HTTP context available? We're starting to see it because the data gathered in RASP doesn't necessarily require a HTTP context (where as most WAF data does).
NachoEchevarria
commented
1 week ago Under such circumstances, we should capture the exception and return null, same as we do in that method when the context is null.
So this method only occurs if there's no HTTP context available? We're starting to see it because the data gathered in RASP doesn't necessarily require a HTTP context (where as most WAF data does).
The error would occur when there is no valid context. For instance, if the request has finished and the context is not valid. The method Uninitialize() can be called to reset the context and reuse it for a new query.
NachoEchevarria
commented
1 week ago This feels like a case of addressing the symptoms instead of the cause, and we see many more symptoms of this 🤔 My real concern is why are we accessing the
HttpContextafter the request has finished? AFAIKUninitialize()is only called after the aspnetcore pipeline has completely finished, so I think we need to work out when and where that is happening? 🤔
We are instrumenting methods that access files. These methods might be called after the request has finished in some cases. If a file operation is performed, we will be called even if the request is finished or we are in a console app. I wonder if maybe we can use rootSpan.IsFinished as a way to check if the request has finished. AFAIK, if the request has finished, the root span should have finished as well. Still, I think that we should be protected against this situations. Maybe write a warning message? We should filter our instrumentations as much as possible to avoid these cases anyway.
andrewlock
commented
1 week ago AFAIK, if the request has finished, the root span should have finished as well
Normally, but not necessarily, e.g. customers could create "root" spans that are the root of all requests 🤔
Still, I think that we should be protected against this situations
I agree 100%, but I think a preferable approach is to not "assume" there is an HttpContext 🤔 Fundamentally it's unsafe to access HttpContext outside of that context (+ HttpContext is not thread safe) so we'll potentially get all sorts of errors, not just ObjectDisposedExceptions (we already have a bunch of NullReferenceExceptions related to this pattern too)
Simply put, I think we need to find a completely different mechanism for attaching this data if we don't know whether an HttpContext is available (rather than "access it and see what happens") 🤔
Maybe write a warning message
This seems like one of the cases where it could happen a lot, and we end up with a lot of warnings. Plus it's not clear that the customer can do anything about it - it's an error on our part, right? - so I'm not sure it adds much value
NachoEchevarria
commented
5 days ago AFAIK, if the request has finished, the root span should have finished as well
Normally, but not necessarily, e.g. customers could create "root" spans that are the root of all requests 🤔
Still, I think that we should be protected against this situations
I agree 100%, but I think a preferable approach is to not "assume" there is an
HttpContext🤔 Fundamentally it's unsafe to accessHttpContextoutside of that context (+HttpContextis not thread safe) so we'll potentially get all sorts of errors, not justObjectDisposedExceptions (we already have a bunch ofNullReferenceExceptions related to this pattern too)Simply put, I think we need to find a completely different mechanism for attaching this data if we don't know whether an
HttpContextis available (rather than "access it and see what happens") 🤔Maybe write a warning message
This seems like one of the cases where it could happen a lot, and we end up with a lot of warnings. Plus it's not clear that the customer can do anything about it - it's an error on our part, right? - so I'm not sure it adds much value
After thinking about it, I have added more checks to ensure that we are inside a request without actually accessing the httpContext. Since I was not able to reproduce this particular error, we will need to see how this change affects to this particular exception. If the exception is thrown, it will still be reported as an error.
NachoEchevarria
commented
4 days ago Thanks for your comments and reviews!
Summary of changes
We were getting a ObjectDisposedException error in the SecurityCoordinator.
In RASP (and IAST), we are instrumenting methods that can be called out of a request context, even when running web apps. For instance, we are instrumenting methods of the class File.
This error occurs when we try to access Context.Features and that field is null. The class DefaultHttpContext checks for null values in that property and launches an exception when it's null. This can happen, for instance, if the method Uninitialize() from DefaultHttpContext is called.
While it was not possible to reproduce this particular exception, some checks were added that could prevent this from happening again. These checks include checking if the additive context has been disposed, if the root span has been closed or if we are not running in a web context. In all of these cases, RASP is not required to run.
Reason for change
Implementation details
Test coverage
Other details