Created attachment 343555 [details] [review]
Abort session after a new proxy resolver is set

I dunno about this one. The current behavior is clearly intentional, but I would think that you would always want an abort after changing the proxy settings to ensure nothing gets sent with the old settings?

(In reply to Michael Catanzaro from comment #2)
> I dunno about this one. The current behavior is clearly intentional, but I
> would think that you would always want an abort after changing the proxy
> settings to ensure nothing gets sent with the old settings?

It could be clearly intentional for you, but Dan told me on IRC that it wasn't, and that we always want to abort when changing proxy settings

I think Dan must have intended it to be this way at the time the code was written:

	case PROP_PROXY_URI:
		set_proxy_resolver (session, g_value_get_boxed (value),
				    NULL, NULL);
		soup_session_abort (session);
		socket_props_changed = TRUE;
		break;
	case PROP_PROXY_RESOLVER:
		set_proxy_resolver (session, NULL, NULL,
				    g_value_get_object (value));
		socket_props_changed = TRUE;
		break;

It would be pretty much impossible to just forget to call soup_session_abort() there, when you can see it's there just a couple lines up. And the documentation indicates the difference as well.

(But again, I have no clue why the current behavior would be desirable.)

Comment on attachment 343555 [details] [review]
Abort session after a new proxy resolver is set

Yeah, I think it ought to abort. Probably the only reason it doesn't is that I assumed people were only ever going to set that property at construct time anyway...

Really, all we need to do is ensure that all currently-open connections get closed after their current request completes (so we don't keep using a non-proxied or incorrectly-proxied connection for an arbitrarily long time after changing the proxy resolution rules). However (a) there is no function that does that, and (b) we assume people aren't going to change their proxy resolver a lot anyway. So, we just abort, because it's simpler.

This caused regression filled as bug #781590.