[breaking change] HttpClient: change StateError to RedirectException when redirect response has no Location header

[brianquinlan]

Change Intent

Currently, HttpClient throws a StateError when the server responds with a 302 but there is no Location header.

Instead, RedirectException (a subclass of HttpException) should be thrown.

See #53158

Justification

StateError should be thrown in response to programmer errors, not errors external to the application. Also, HttpException subclasses are thrown in response to other HTTP protocol errors.

Some users (e.g. package:http) assume that HttpClient will only throw SocketException or HttpException.

Impact

This will break HTTP client code that catches StateError but not HttpException and interacts with HTTP servers that return 302 but don't include a Location header. This seems unlikely to happen in practice.

Mitigation

It is likely that no actual code will need to be migrated. In the worst case, an exception type in a on clause might need to change.

Change Timeline

N/A.

Associated CLs

No response

[lrhn]

Technically not breaking to change a thrown error to something else, but if it's the only signal we've given people, for something which ought to have been an exception, it's likely it's been treated like an exception.

[brianquinlan]

Technically not breaking to change a thrown error to something else, but if it's the only signal we've given people, for something which ought to have been an exception, it's likely it's been treated like an exception.

How is that not breaking? If my current code is:

do {
  try {
    HttpClientRequest request = await client.get('brokenserver.com?token=$token', 80, '/');
  } on StateError {
    // brokenserver.com serves broken redirects if the auth token has expired, refetch it
    token = reauth(...);
    continue;
  }
  break;
} while (true)

Then I will break because I'm handling StateError rather than RedirectException.

[itsjustkevin]

@brianquinlan based on @lrhn comment and the context you provided in the following comment, I assume we are still considering this as a breaking change to go through the approval process. Am I misreading this?

[lrhn]

@brianquinlan
Technically (always a good start) a "breaking change" isn't just any change that makes a hypothetical program change behavior.
We've tried to define it as not breaking well behaved programs.

If some program starts using private API from lib/src/, or does things the documentation clearly says not to do, then it getting broken is it's own damn fault.
One of the things we have listed as not being guaranteed behavior is errors.
We reserve the right to make code that currently throw an Error behave differently, either throwing a different error, or succeeding. An error is intended to report a programming error, and currently unsupported operation, so a program which depends on a specific error being thrown is not "well behaved".

In this particular case, because we probably should have made it an exception to begin with, and users have had no alternative to handle the exceptional situation other than to catch the error, we will treat it as if it has been an exception.

(A less breaking change could be to throw an object which implements both RedirectException and StateError, document it as only being the former, and maybe start hinting to change catches of the latter that we can detect statically.)

[brianquinlan]

@itsjustkevin - based on @lrhn's comment, I'm not sure ;-)

It seems safer to follow the process and deciders can just give a quick +1 if they don't consider this change to be breaking ;-)

[itsjustkevin]

Got it @brianquinlan. Thoughts on this @Hixie @vsmenon @grouma?

[brianquinlan]

Ping for @Hixie, @vsmenon, @grouma

[grouma]

It is likely that no actual code will need to be migrated. In the worst case, an exception type in a on clause might need to change.

How do we intend to surface this problematic code?

[brianquinlan]

It is likely that no actual code will need to be migrated. In the worst case, an exception type in a on clause might need to change.

How do we intend to surface this problematic code?

I don't know if there is a way to do that.

[lrhn]

If we want to be non-breaking, while still pushing people towards the exception, we can introduce a chimera-exception:

class _RedirectException extends RedirectException implements StateError {
  _RedirectException(super.message, super.redirects);
}

and throw that. Then document as throwing RedirectException, put a breaking change notice into the change-logs (even if it isn't breaking yet), write in big letters that it will no longer throw a StateError in all the docs.

Then wait and see. Maybe try to remove the implements StateError occasionally and see if it gets through G3.

Eventually, do the real breaking change.

[brianquinlan]

If we want to be non-breaking, while still pushing people towards the exception, we can introduce a chimera-exception:

class _RedirectException extends RedirectException implements StateError {
  _RedirectException(super.message, super.redirects);
}

and throw that. Then document as throwing RedirectException, put a breaking change notice into the change-logs (even if it isn't breaking yet), write in big letters that it will no longer throw a StateError in all the docs.

Then wait and see. Maybe try to remove the implements StateError occasionally and see if it gets through G3.

Eventually, do the real breaking change.

We've used this approach before but it seems pretty high effort considering how unlikely this is to break real code.

[Hixie]

no objections from me

[itsjustkevin]

Pinging @grouma and @vsmenon again.

[grouma]

SGTM.

[itsjustkevin]

Pinging @vsmenon for final review.

[vsmenon]

lgtm