http library throws `HttpException: Connection closed before full header was received` without a stack trace

[andrewkolos]

Parent issue: https://github.com/dart-lang/sdk/issues/44994.

See https://github.com/flutter/flutter/issues/153298, which is currently (as of 8/14/24) one of top crashers of the Flutter CLI tool. In this crash we see exceptions of the form HttpException: Connection closed before full header was received, uri = http://localhost:50558/devtools/page/31E436435F75FDDC3F156C20242E9ECD. However, these exceptions come without stack traces, making them difficult to troubleshoot (https://github.com/flutter/flutter/issues/153298#issuecomment-2286484496).

grepping for the message, I see two possible sources:

https://github.com/dart-lang/sdk/blob/aca875dfea35e02c3bacd15375dc285a589081f1/sdk/lib/_http/http_parser.dart#L909-L910 and/or https://github.com/dart-lang/sdk/blob/aca875dfea35e02c3bacd15375dc285a589081f1/sdk/lib/_http/http_parser.dart#L925-L926

Might it be possible to preserve some sort of stack trace here? This could make it easier for http library users to figure out where these exceptions are coming from.

[dart-github-bot]

Summary: The http library throws an HttpException with the message "Connection closed before full header was received" without a stack trace, making it difficult to debug the source of the error. Adding a stack trace to this exception would improve debugging for users.

[mraleph]

We could record a stack trace at the point where HTTP request is made and then use that stack trace when reporting http exceptions. A more interesting consideration could be to tweak semantics of how exceptions propagate: @lrhn could we change language semantics that rethrowing an exception without a stack trace attaches the current stacktrace to it? Would that be bad somehow?

[lrhn]

could we change language semantics that rethrowing an exception without a stack trace attaches the current stacktrace to it?

Which "current stacktrace"? The currently caught one or the one at the rethrow? I'm guessing the former, the one that is thrown along with the rethrow. That is: Doing a rethrow when the caught object extends Error and has no attached Error.stackTrace will attach the caught stack trace to the Error object, just like throwing it the first time with that stack trace would.

You can do Error.throwWithStackTrace(error, stackTrace), which will set the stack trace on the error if it can, so the functionality is available, so you're asking for rethrow to behave the same way as that, and the same way as a throw would for the same object and "current" stack trace.

It doesn't feel right. Or rather, it feels like it shouldn't be necessary, so doing it would be patching over the real problem. A rethrow always re-throws an object that was already thrown once (since it was caught), so it should have a stack trace attached to Error.stackTrace.

I guess some paths through async code does not set the stack trace. A Future.error(StateError("Q"), StackTrace.fromString("S")) will eventually be able to raise that error and stack trace as something to catch, but it never actually throw the error, so the stack trace is not attached to the error.

I'd be fine with changing that, giving dart:async the ability to try setting the stack trace on an Error-extending object, if there is none, probably an external void _trySetStackTrace(Error error, StackTrace stack), and then use that in the places where an error object is passed into async from the outside. Probably right after every place we call Zone.errorCallback. Then there should be no caught Error with no stack trace set, so rethrow wouldn't need to do anything. I'd rather do that than make rethrow having to do more work.

(So I did. Not done yet, tests do not run, so I don't know where the bug is yet. There is some design work to do. For example, should Future.error(e, s) set the stack on e even if Zone.errorCallback intercepts the error and replaces it with AsyncError(e2, s2)? Or should the stack s2 be set on e2 then? Most likely the latter, since that's the object that will eventually be caught. Should it also set s on e, just for good measure? Since that's what the author thought they were throwing? ... Probably not. Most reasonable uses of errorCallback won't change the error object, but may change the stack, at which point we do want to set the new stack on the existing object, which we can't if we already set the original stack on it. Also have to decide which user provided errors count as introducing new errors. The Zone.errorCallback invocation is probably the most precise signal, because that's precisely where we consider an object to go from being synchronous data to asynchronous error. Just have to make sure no internal async implementation code bypasses that. I put it in _Future._setError instead, at the final point of completing the future with an error. Will have to check if that makes a difference.)

[a-siva]

//cc @brianquinlan