Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix multi callback issues: easy.pause(), easy.close(), multi.close() #708

Merged
merged 3 commits into from
Jan 11, 2022

Conversation

fsbs
Copy link
Contributor

@fsbs fsbs commented Oct 12, 2021

This ensures callbacks invoked by some easy-related functions have
access to the corect thread state.

Fixes callbacks invoked from:

  • do_curl_perform
  • do_curl_pause
  • util_curl_xdecref

This is the fix I referred to in #592 (comment), a continuation of #594. The latter only fixed callbacks invoked by multi handles. This one fixes them when they're invoked by easy handles as well.

Practical example:

  1. set socket and timer callback
  2. add easy handle
  3. perform a chunk of the transfer
  4. pause the easy handle (do_curl_pause)

At this point callbacks will be invoked to let you know that you should stop polling for sockets associated with that easy handle. But only easy->state is set by PYCURL_BEGIN_ALLOW_THREADS, so the multi handle in multi_socket_callback() won't have access to that state and will fail to acquire thread.

Thus we need to set a reference to easy->multi_stack->state as well. For that purpose I've added PYCURL_BEGIN_ALLOW_THREADS_EASY and placed it at those points where easy handles can invoke callbacks.

The practical consequence of this fix is that event-driven transfers (such as asyncio) now work properly when easy handles are paused, unpaused or closed mid-transfer.

Previously multi_socket_callback() would silently fail, while your event loop would go on polling sockets that are not active anymore - or even worse (when easy handle is unpaused), the event loop would never learn about the sockets that it should start polling.

This ensures multi's callbacks invoked by some easy functions have
access to thread state.

Fixes callbacks invoked by:
 * do_curl_pause
 * util_curl_xdecref
@fsbs
Copy link
Contributor Author

fsbs commented Oct 12, 2021

The above build failed because of the assert on line 22:

if (self->state != NULL)
{
/* inside perform() */
assert(self->handle != NULL);
if (self->multi_stack != NULL) {
assert(self->multi_stack->state == NULL);
}
return self->state;
}
if (self->multi_stack != NULL && self->multi_stack->state != NULL)
{
/* inside multi_perform() */
assert(self->handle != NULL);
assert(self->multi_stack->multi_handle != NULL);
assert(self->state == NULL);
return self->multi_stack->state;
}

The assert logic is OK: it verifies that easy.perform() is not called in a different thread at the same time as multi functions - not just multi.perform() though as the comment on line 28 might imply.

Simply setting both easy->state and easy->multi_stack->state at the same time was a really stupid way of fixing easy's callback invocations :) It worked fine for me because my callback tests were using asyncio instead of threads, while pytest uses threads.

I've now changed PYCURL_BEGIN_ALLOW_THREADS_EASY to this:

  • if easy->multi_stack == NULL, then easy->state = state
  • else easy->multi_stack->state = state

For easy.perform() I've also reverted back to using PYCURL_BEGIN_ALLOW_THREADS because even though call graph might show it can invoke multi's callbacks, that's not the correct way to use libcurl anyway: you have to remove an easy handle from multi before calling easy.perform() - and multi.remove_handle() has already been fixed by #594. So this fix now concerns only easy.pause() and easy.close().

pytest passes on my end now so I'll push the fix.

@fsbs fsbs changed the title Sync thread state of easy and easy->multi_stack Set easy->multi_stack->state for multi's callbacks Oct 12, 2021
Tests invocation of callbacks by the following methods:

 * multi.socket_action(...)
 * multi.add_handle(easy)
 * [mid-transfer] multi.remove_handle(easy)
 * [mid-transfer] easy.pause(PAUSE_ALL)
 * [mid-transfer] easy.pause(PAUSE_CONT)
 * [mid-transfer] easy.close()
@fsbs
Copy link
Contributor Author

fsbs commented Oct 20, 2021

I've written tests for SOCKETFUNCTION and TIMERFUNCTION. The tests cover issues that #594 and this patch fixes.

Good news is that they caught a bug which happens when you close multi handle mid-transfer without removing easy handles from it first.

I'm adding the fix and tests to this PR since it's related.

@fsbs fsbs changed the title Set easy->multi_stack->state for multi's callbacks Fix multi callback issues: easy.pause(), easy.close(), multi.close() Oct 22, 2021
@p p merged commit 15d41a4 into pycurl:master Jan 11, 2022
@fsbs fsbs mentioned this pull request Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants