only close curl handle if it's done

[divinity76]

if a handle happens to create another message than "done", then we shouldn't remove the handle yet, right?

notably, there are no other message types in upstream libcurl yet,
but this will help future-proof Guzzle so 2021 versions of Guzzle may run better on future versions of libcurl.

[Nyholm]

Thank you for this PR.

It cannot be merged without tests. Please add some.

[divinity76]

@Nyholm don't think that's currently possible with stock libcurl. the msg member states what kind of message this is, and currently the member only exist to make sure curl_multi_info_read() can return other types of messages in the future without breaking the C api. however, for the time being, the only message type that exist is the CURLMSG_DONE message (ref https://curl.se/libcurl/c/curl_multi_info_read.html ),
there can be no testcases that enter this !==CURLMSG_DONE condition without modifying libcurl, or waiting until upstream libcurl adds other messages in the future. would be possible to add a wrapper for curl_multi_info_read like

// curl_multi_info_read wrapper which also returns a fake message to test if other code correctly handles
// a message where ["msg"] !== CURLMSG_DONE
function curl_multi_info_read_with_fake_test_data($multi_handle, int &$queued_messages = null){
    static $has_delivered_fake_message = false;
    if(!$has_delivered_fake_message){
        $has_delivered_fake_message = true;
        $queued_messages = 1; //
        return array(
            'msg'=>CURLMSG_DONE + 1, // << fake message id, does not exist in libcurl (yet?)
            'result' => CURLM_INTERNAL_ERROR, // << can be anything, just a fake test message
            'handle' => null, // << ??
            );
    }
    return curl_multi_info_read($multi_handle, $queued_messages);
}

though..

however, if Guzzle waits until libcurl actually starts returning other messages than CURLMSG_DONE before adding code to handle it, then Guzlle will just randomly start breaking one day because Guzzle treats all messages as if it was a CURLMSG_DONE message, even when it isn't...

[GrahamCampbell]

I'm hesitant to merge this since there's no example where this is broken, and there are no tests. Please feel free to open a new PR if you feel strongly differently. Guzzle is very widely used, and we have to be very careful not to break things for people. :)

[bagder]

Can I object to the closing of this PR? I'm the main author of libcurl and I wrote this API. It is clearly documented to return a CURLMSG_DONE for this case and only then does it mean that a handle/transfer is complete. Not checking for that value works only by chance and will break in the case libcurl adds another message.