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

Jump to bottom

Curl_conncache_remove_conn: Assertion `!(data)->state.conncache_lock' failed #4029

Closed
kunalekawde opened this issue Jun 16, 2019 · 7 comments
Closed

Curl_conncache_remove_conn: Assertion `!(data)->state.conncache_lock' failed #4029

kunalekawde opened this issue Jun 16, 2019 · 7 comments
Assignees
@bagder

Comments

@kunalekawde
Copy link

kunalekawde commented Jun 16, 2019 •
edited

I did this

The test case involves sequential HTTP transfer with timeout period in between which should result in closing existing connection. Its using CURLOPT_SHARE and CURLDEBUG(debug build:--enable-debug --enable-curldebug).

Following is the sample program to simulate the issue:
https://gist.github.com/kunalekawde/fa8915cf5dbcac3148501119e30d8f63

I expected the following

Sequential HTTP transfers to complete without assertion.

curl/libcurl version

curl 7.65.1
Since the issue was seen in master, I could isolate it to 1 fix after 7.65.1
f67009d for the pull: #4013

To reproduce please use: 7.65.1 + 4013(as mentioned above), other latest from curl master can also be used.

operating system

Fedora 28

observation

As with debug build, there are assertions during CONN_LOCK and CONN_UNLOCK for double lock and lock/unlock without existing unlock/lock on same respectively.
After a lot of playing around, learnt that its when CURLOPT_SHARE is used along with debug build.
In this testcase flow its observed at connection is being locked during disconnection due to max age of the connection which is already holding the lock.

fix

https://gist.github.com/kunalekawde/34c73ee19aabd2281c2a2abd915ea00a

Although the fix is working for the case there could be better or other places affected, request to please address or if the fix is fine, let me know if should pull this to master.
@bagder bagder self-assigned this Jun 16, 2019
bagder added a commit that referenced this issue Jun 16, 2019
@bagder
conn_maxage: move the check to prune_dead_connections()
01b5e6e 
... and avoid the locking issue.

Reported-by: Kunal Ekawde
Fixes #4029
@bagder bagder mentioned this issue Jun 16, 2019
Closed
@bagder
Copy link
Member

bagder commented Jun 16, 2019

Thanks, although I propose a different fix in #4032 that I think actually simplifies the code overall.

@kunalekawde
Copy link
Author

kunalekawde commented Jun 17, 2019 •
edited

Thanks @bagder but with the fix in #4032, I don't see connection being torn down("too old connection") after timeout(tried with 220 sec) . Is it correct ?

@bagder
Copy link
Member

bagder commented Jun 17, 2019

Thanks. Ok, let me write up a test case that verifies the correct behavior and I'll check. Maybe I messed up here...

@bagder
Copy link
Member

bagder commented Jun 17, 2019

My little example maxage-conn.c seems to work as presumed both with and without the #4032 PR. I set the maximum age to one second and after a two second sleep the next transfer won't reuse the connection since it is now too old.

@kunalekawde
Copy link
Author

kunalekawde commented Jun 17, 2019 •
edited

Above example doesn't seem to have CURLOPT_SHARE, would it be possible for you to try a run with example I provided in initial request?
https://gist.github.com/kunalekawde/fa8915cf5dbcac3148501119e30d8f63

@bagder
Copy link
Member

bagder commented Jun 17, 2019

  1. That example doesn't even use CURLOPT_MAXAGE_CONN ?
  2. The example shares cookies in the share object, not connections.
  3. The code that scans the connections to kill idle ones work the same with shared or non-shared connection pool
  4. When I edit my example to use a shared object with connections for me, it works: https://gist.github.com/bagder/e0dd337453665dd259c008f3df14bb95

PS the test 188 failures we see in my #4032 PR is an unrelated bug that's being fixed in #4034

@kunalekawde
Copy link
Author

kunalekawde commented Jun 17, 2019 •
edited

mybad, subsequent program to test was not linking to correct library, verified with correct one, so we are good. Thanks.

bagder added a commit that referenced this issue Jun 17, 2019
@bagder
conn_maxage: move the check to prune_dead_connections()
43623a8 
... and avoid the locking issue.

Reported-by: Kunal Ekawde
Fixes #4029
Closes #4032
@bagder bagder closed this as completed in 755083d Jun 17, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bagder bagder

Labels
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

conn_maxage: move the check to prune_dead_connections() curl/curl
2 participants
@bagder @kunalekawde