I'm not a fan of the commit title, it reads more like an explanation in a commit message. http2: mark the connection for close on GOAWAY is easier to understand. Also it's unclear to me if marking it for close can through some effect (?) prematurely terminate the connection and disrupt established streams. If I understand correctly server sent GOAWAY xx isn't necessarily the end it just means no more streams are being processed past whatever xx is, but <=xx may have yet to be served

I like your suggested commit message fix, I'll change.

Also it's unclear to me if marking it for close can through some effect (?) prematurely terminate the connection and disrupt established streams

Even if this fix isn't a 100% fix for GOAWAY, it still improves the situation since before this PR, libcurl treated the GOAWAY as an error and stopped. Now it "only" marks the connection for close, which is much more likely to be right.

I totally agree that it leaves a risk that if we have stream A and B going, and we receive a GOAWAY, we should still allow both streams to terminate before we close the connection and I believe the current setup might now close the connection once the next stream ends instead of allowing all existing streams to end.

It would be nice to be able to setup a test that does exactly that...

Hm, actually I think it should be fine. should_close_session should not return TRUE until both streams are done.

should_close_session should not return TRUE until both streams are done.

No, that's not true either I think. It only returns false if there's data currently in the queues to "drain" for any stream. A complete fix will probably need to check if there's a non-closed stream still present on the connection before it allows it to get closed. It should still be marked for closure immediately on the receive of GOAWAY as it then makes sure the connection is not used further for new streams.

We already had the check in multi_done, it just wasn't entirely correct since it would let bits.close override it, which seems wrong. I've forced-pushed an update now with that part addressed as well.