Responding to the proposed code changes:

> most importantly
> both cases should (must?) make curl_multi_perform() return
> CURLM_CALL_MULTI_PERFORM.

I agree in both cases curl_multi_perform() should return
CURLM_CALL_MULTI_PERFORM. I don't think that matters for me, the way my
application is coded, but you are right, to keep the semantics of
curl_multi_perform() correct, it should return CURLM_CALL_MULTI_PERFORM.

> I also think both these changes are slightly too naive and
> simple-minded in
> the way they just return instead of keeping some kind of
> state to be able to
> get back to where they were in the next invoke

I looked at that and it appeared to me that the state was already saved, and
that the next time curl_multi_perform() was called, it would (eventually)
return to the right place.

> > while (easy->easy_handle->change.url_changed);
> >
> > while (0);
> >
> > This is a minor change. change.url_changed is only true when the
> > application sets CURLOPT_URL while the connection is in
> progress, but in
> > that one case, this change will ensure read is not called
> until the next
> > time the application calls curl_multi_perform.
 
> This was added to make libcurl properly deal with the (very
> rare) situation
> when you get the new URL during the SSL handshake and the
> remote cert would
> indicate the page to get. I don't currently recall why it was
> important to
> allow libcurl to do the magic itself instead of leaving it to
> the app, but I
> can dig up the reasoning if need be.

I note that the only time this change could possibly present an issue is if,
when the program reaches the bottom of the loop, both change.url_changed and
CURLM_STATE_COMPLETED==easy->state (which is the next and only thing tested
after the loop) are both true. Is it possible for change.url_changed to be
true when easy->state==CURLM_STATE_COMPLETED?

> > CHANGE:
> >
> > ((select_res & CSELECT_IN) || conn->bits.stream_was_rewound)) {
> >
> > TO:
> >
> > ((select_res & CSELECT_IN) || conn->bits.stream_was_rewound) ||
> > data_pending(conn)) {
>
> [...]
>
> > This moves the test for data_pending(conn) out of the loop. As a
> > result,
> > curl_multi_perform will not loop back to repeat the read when
> > data_pending(conn) is true, but instead it will return to
> the application
> > and then call read the next time it is called.
 
> Eh, no. Taking out that function will effectively ruin how
> libcurl works with
> both OpenSSL and libssh2 as both those libs are sucking in
> data by themselves
> to a buffer, so we cannot simply just wait for the socket to
> become readable
> again to know if there is more data to read from them.

Yes, the application cannot wait for the socket to become readable again,
because there is data sitting in the SSL decode buffer. If
curl_multi_perform() returns CURLM_CALL_MULTI_PERFORM then the application
should call curl_multi_perform() again without testing the socket.

curl_multi_perform() should also restart the loop without waiting on the
socket. That is why the data_pending(conn) was placed at the top of the
loop, to ensure this will happen.

One troubling thing in the code is that there are a few functions like sftp
that call Curl_read but only transfer.c appears to check data_pending(conn).
How do those functions deal with data in the SSL buffer? Since I'm not
using these functions, I didn't look into this further.

I understand your concern with saving state and side effects. The changes
were not made naively--I looked at the code for those issues first and
didn't see any. That is not to say however that I caught all possible
effects--you are much more familiar with the code.

Allen
Received on 2007-06-22