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

multi: support verbose and err stream for conncache closure handle #3618

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
3 participants
@jay
Copy link
Member

commented Feb 26, 2019

  • Change closure handle to receive verbose and error stream settings
    from the easy handle most recently added via curl_multi_add_handle.

The closure handle is a special easy handle used for closing cached
connections. It receives limited settings from the easy handle most
recently added to the multi handle. Prior to this change that did not
include verbose and the error stream which was a problem because on
connection shutdown verbose mode was not acknowledged.

Ref: #3598

Closes #xxxx

@bagder

This comment has been minimized.

Copy link
Member

commented Feb 26, 2019

The test 1506 output changes seem legit due to the new verbosity.

@jay

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2019

I'm having second thoughts. Does anyone think it will be a problem to copy the stderr FILE stream from the easy handle? What if the handle is removed and the stream is closed or fd reused? That worries me.

@bagder

This comment has been minimized.

Copy link
Member

commented Feb 28, 2019

Yes I agree. The verbosity level I think is fairly safe but having the stderr pointer survive and "surprise" users after the original transfer is gone I think might lead to unhappiness.

multi: support verbose conncache closure handle
- Change closure handle to receive verbose setting rom the easy handle
  most recently added via curl_multi_add_handle.

The closure handle is a special easy handle used for closing cached
connections. It receives limited settings from the easy handle most
recently added to the multi handle. Prior to this change that did not
include verbose which was a problem because on connection shutdown
verbose mode was not acknowledged.

Ref: #3598

Closes #xxxx

@jay jay force-pushed the jay:closure_handle_verbose branch from 6663aec to 6337014 Mar 2, 2019

@jay

This comment has been minimized.

Copy link
Member Author

commented Mar 2, 2019

test still fails because the order isn't always the same

-* Closing connection 3[CR][LF]
 * Closing connection 1[CR][LF]
+* Closing connection 3[CR][LF]
 * Closing connection 2[CR][LF]

I'm trying strip ^\* Closing connection (?:1|2|3)$ rather than mark it flaky. The 'Closing connection 0' position is needed for the test AFAICS so I can't strip wholesale.

@bagder

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

You need to put that strip in the <stripfile> section, and I propose only stripping out the number so we still verify the number of lines. Like this:

diff --git a/tests/data/test1506 b/tests/data/test1506
index 9e440907e..34a72cdf6 100644
--- a/tests/data/test1506
+++ b/tests/data/test1506
@@ -79,19 +79,22 @@ Host: %HOSTIP:%HTTPPORT
 Accept: */*
 
 </protocol>
 <strip>
 ^Host:.*
-^\* Closing connection (?:1|2|3)$
 </strip>
 <file name="log/stderr1506" mode="text">
 * Connection #0 to host server1.example.com left intact
 * Connection #1 to host server2.example.com left intact
 * Connection #2 to host server3.example.com left intact
 * Closing connection 0
 * Connection #3 to host server4.example.com left intact
+* Closing connection
+* Closing connection
+* Closing connection
 </file>
 <stripfile>
 $_ = '' if (($_ !~ /left intact/) && ($_ !~ /Closing connection/))
+s/^(\* Closing connection) [123]/$1/
 </stripfile>
 </verify>
 </testcase>

bagder and others added some commits Mar 3, 2019

@jay jay closed this in b0972bc Mar 5, 2019

@jay jay deleted the jay:closure_handle_verbose branch Mar 5, 2019

jay added a commit to jay/curl that referenced this pull request May 9, 2019

Revert "multi: support verbose conncache closure handle"
This reverts commit b0972bc.

- No longer show verbose output for the conncache closure handle.

The offending commit was added so that the conncache closure handle
would inherit verbose mode from the user's easy handle. (Note there is
no way for the user to set options for the closure handle which is why
that was necessary.) Other debug settings such as the debug function
were not also inherited since we determined that could lead to crashes
if the user's per-handle private data was used on an unexpected handle.

The reporter here says he has a debug function to capture the verbose
output, and does not expect or want any output to stderr; however
because the conncache closure handle does not inherit the debug function
the verbose output for that handle does go to stderr.

There are other plausible scenarios as well such as the user redirects
stderr on their handle, which is also not inherited since it could lead
to crashes when used on an unexpected handle.

Short of allowing the user to set options for the conncache closure
handle I don't think there's much we can safely do except no longer
inherit the verbose setting.

Bug: https://curl.haxx.se/mail/lib-2019-05/0021.html
Reported-by: Kristoffer Gleditsch

Ref: curl#3598
Ref: curl#3618

Closes #xxxx

jay added a commit that referenced this pull request May 10, 2019

Revert "multi: support verbose conncache closure handle"
This reverts commit b0972bc.

- No longer show verbose output for the conncache closure handle.

The offending commit was added so that the conncache closure handle
would inherit verbose mode from the user's easy handle. (Note there is
no way for the user to set options for the closure handle which is why
that was necessary.) Other debug settings such as the debug function
were not also inherited since we determined that could lead to crashes
if the user's per-handle private data was used on an unexpected handle.

The reporter here says he has a debug function to capture the verbose
output, and does not expect or want any output to stderr; however
because the conncache closure handle does not inherit the debug function
the verbose output for that handle does go to stderr.

There are other plausible scenarios as well such as the user redirects
stderr on their handle, which is also not inherited since it could lead
to crashes when used on an unexpected handle.

Short of allowing the user to set options for the conncache closure
handle I don't think there's much we can safely do except no longer
inherit the verbose setting.

Bug: https://curl.haxx.se/mail/lib-2019-05/0021.html
Reported-by: Kristoffer Gleditsch

Ref: #3598
Ref: #3618

Closes #3856
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.