NSURLSession fixes and improvements #286

triplef · 2023-01-13T12:09:05Z

Adds various missing APIs on NSURLSession (some of them stubs calling -notImplemented:)
Fixed various memory management issues
- -[GSEasyHandle resetTimer] was referencing a destroyed object
- libdispatch objects were not being released
- GSHTTPURLProtocol was over-releasing an array object
Fixed various cases where NSURLSession header fields were not being treated case-insensitive, because classes were accidentally using normal NSDictionary objects instead of _GSMutableInsensitiveDictionary e.g. when being copied.

rfm

Looks good generally, but in -resetTimer it looks like the lines got out of order (so DESTROY is called on the new timer rather than the old one).
The only other issue I see is cosmetic; coding style is that curly braces are opened on the line after an 'if' and are indented by two spaces.

rfm

looks goo. Thanks.

triplef · 2023-01-13T12:41:26Z

Ah yes, good catch thank you! -resetTimer and also the if statement style should be fixed now.

rfm

Generally, I think this looks fine (and new methods don't hurt anything). But a few style notes:

The convention in the base library is that method names should be in alphabetical order so they are easy to find (we don't put setter/getter together).
An unimplemented method returning an object conventionally returns the result of calling the -notImplemented: method.

triplef · 2023-01-13T14:47:16Z

The convention in the base library is that method names should be in alphabetical order so they are easy to find (we don't put setter/getter together).

I wasn’t aware of that, and it doesn’t seem to be documented here. It also doesn’t seem like this is followed much at all in the existing headers – just searching for ) set in the existing headers it seems to me that most of them do put getter/setter together, which I find much more intuitive and seems like others followed that as well. I try to match the style as much as possible to what I see, that’s why I changed these in NSURLSessionConfiguration.

I’ll fix the order so it’s alphabetical, let me know if you want me to revert it to the previous separated getters/setters as well.

An unimplemented method returning an object conventionally returns the result of calling the -notImplemented: method.

Good point, I didn’t realize it returned id. I’ll fix that.

rfm · 2023-01-13T15:36:26Z

On 13 Jan 2023, at 14:47, Frederik Seiffert ***@***.***> wrote: • The convention in the base library is that method names should be in alphabetical order so they are easy to find (we don't put setter/getter together). I wasn’t aware of that, and it doesn’t seem to be documented here. It also doesn’t seem like this is followed much at all in the existing headers – just searching for ) set in the existing headers it seems to me that most of them do put getter/setter together, which I find much more intuitive and seems like others followed that as well. I try to match the style as much as possible to what I see, that’s why I changed these in NSURLSessionConfiguration. I’ll fix the order so it’s alphabetical, let me know if you want me to revert it to the previous separated getters/setters as well.

I agree it's not documented there, which might be part of the reason that it's not followed too much, though I think you will find that its followed more than any alternative. Another might be that I don't enforce it. Mostly I find that if it's not done alphabetically we get grouping of methods based on the current focus of whoever happens to be making a change (which, over time, becomes pretty random). i actually quite like getters and setters to be together, so maybe we should see if we can get an agreement and update the coding standards to make an explicit convention like "methods in a class, category or protocol are arranged in alphabetic order *except* that any setter method -setSomething: should immediately follow the corresponding getter method -something"

triplef · 2023-01-13T15:53:10Z

"methods in a class, category or protocol are arranged in alphabetic order except that any setter method -setSomething: should immediately follow the corresponding getter method -something"

Sounds perfect to me.

I actually found some more issues with the completionHandler implementation I added, so there will be further updates to this.

fredkiefer · 2023-01-13T17:19:15Z

The convention in the base library is that method names should be in alphabetical order so they are easy to find (we don't put setter/getter together).

Does this apply for both the header and the implementation file?

rfm · 2023-01-13T17:38:35Z

On 13 Jan 2023, at 17:19, Fred Kiefer ***@***.***> wrote: — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because your review was requested.

I try to do it in both. Not obsessive about it though, and (as a special case) I'd kind of like to place setters immediately after the corresponding getters. I just haven't been doing that because, nobody seems to do it consistently (and the alphabetical order convention seems the most/only common one).

triplef · 2023-01-13T18:27:56Z

I’ve tracked down another issue in the NSURLSession implementation and I was wondering if you had any thoughts on how to fix it.

It seems like when the first handle is added to GSMultiHandle, -addHandle: implicitly calls timeoutTimerFired with the special socket CURL_SOCKET_TIMEOUT:

libs-base/Source/GSMultiHandle.m

Lines 142 to 148 in 8ea4c32

    
           // If this is the first handle being added, we need to `kick` the 
        
           // underlying multi handle by calling `timeoutTimerFired` as 
        
           // described in 
        
           // <https://curl.haxx.se/libcurl/c/curl_multi_socket_action.html>. 
        
           // That will initiate the registration for timeout timer and socket 
        
           // readiness. 
        
           BOOL needsTimeout = false;

The problem is that this special (invalid) socket number is then used to create a libdispatch source via the following calls, which causes libdispatch to later crash:

I’m not really familiar with libcurl, but I think we might need to check for CURL_SOCKET_TIMEOUT in curl_socket_function() or one of the method it calls and prevent creation of a dispatch source for it. Does that sound right?

triplef · 2023-01-13T18:42:55Z

Never mind, that was a red herring... curl_socket_function() is not being called with socket == CURL_SOCKET_TIMEOUT.

However libdispatch still sometimes crashes here because WSAEventSelect() fails with 10038 (WSAENOTSOCK: "Socket operation on nonsocket"):

https://github.com/apple/swift-corelibs-libdispatch/blob/469c8ecfa0011f5da23acacf8658b6a60a899a78/src/event/event_windows.c#L214-L223

I need to dig into it further...

triplef · 2023-01-16T18:57:55Z

FYI I’ve tracked down the crash I was seeing further and am trying to get some guidance on the curl mailing list:
https://curl.se/mail/lib-2023-01/0054.html

I think the crash might be specific to the Windows implementation of libdispatch, but the general issue remains that our current implementation does not adhere to the requirements of dispatch_source_cancel(), which say the socket must be closed only after the cancellation handler has fired.

rfm · 2023-01-16T19:49:17Z

On 16 Jan 2023, at 18:58, Frederik Seiffert ***@***.***> wrote: FYI I’ve tracked down the crash I was seeing further and am trying to get some guidance on the curl mailing list: https://curl.se/mail/lib-2023-01/0054.html I think the crash might be specific to the Windows implementation of libdispatch, but the general issue remains that our current implementation does not adhere to the requirements of dispatch_source_cancel(), which say the socket must be closed only after the cancellation handler has fired.

So presumably the only way to implement this is to set up a cancellation handler and close the socket in the handler. I'm guessing the problem with that is that something else needs to close the socket before the handler is guaranteed to be called?

triplef · 2023-01-17T11:44:54Z

Yeah unfortunately I don't think closing the socket from the cancellation handler is always possible either, as in some cases the socket is directly closed by libcurl without a hook for the app to handle socket closing. However I found that the issue doesn’t occur with libdispatch on Mac (closing the socket before cancelling the source works fine), so I opened a pull request with libdispatch to ignore the error and am waiting for their feedback.

In the meantime I also wanted to look into extending the NSURLSession tests a bit, and I was wondering: would there be an easy way to extend the SimpleWebServer implementation used by the tests to handle multiple connections? It’d be nice if we could a) test both delegate- and completion handler-based tasks without having to run the run-loop twice, and b) do a bit of stress testing with multiple requests.

…ease in GSHTTPURLProtocol.

…ive.

…d of queue for session identifier Creating stand-alone dispatch queues without a target is discouraged.

Matches the documented behavior.

Now using the previously unused "in-memory" body data drain if a task has a completion handler, which requires the full body to be passed on completion. Also consolidated private NSURLSessionTask methods, some of which were previously implemented twice in separate categories with the same name, leading to possible undefined runtime behavior.

triplef · 2023-02-20T16:18:19Z

@rfm I’d appreciate if you could review this once more, as I had made a bunch of further changes after your initial review.

Regarding the issue with libdispatch on Windows, based on the discussion I had with their maintainers (swiftlang/swift-corelibs-libdispatch#772) for now I’m keeping the libdispatch patch proposed in that PR as part of the tools-windows-msvc toolchain, which should at least fix most instances of that crash. I think a proper fix would require some changes in libcurl as well, but it’s beyond what I’m able to contribute atm. Since the swift-corelibs-foundation library is running into the same issue I’m hoping that someone there will figure out a solution at some point.

triplef · 2023-02-20T16:19:23Z

Btw. here’s a CI run with the necessary tools-windows-msvc changes adding libcurl:
https://github.com/gnustep/libs-base/actions/runs/4225184793

rfm

I finally got enough time to look through this large patch again, and I can't see any problems with it. Thanks a lot for all the work.

triplef · 2023-03-11T19:22:26Z

Thanks @rfm!

triplef requested a review from rfm as a code owner January 13, 2023 12:09

rfm requested changes Jan 13, 2023

View reviewed changes

rfm approved these changes Jan 13, 2023

View reviewed changes

triplef force-pushed the nsurlsession-additions branch from 8ca684c to 39e455a Compare January 13, 2023 12:39

rfm approved these changes Jan 13, 2023

View reviewed changes

triplef marked this pull request as draft January 16, 2023 17:11

triplef force-pushed the nsurlsession-additions branch from 6ef614e to 11dfeba Compare January 16, 2023 18:36

triplef added 12 commits February 17, 2023 10:41

Add missing NSURLSession APIs

05adcf6

Fix NSURLSession memory management of libdispatch objects and overrel…

e3c4335

…ease in GSHTTPURLProtocol.

Fix NSURLSession header fields not always being matched case insensit…

cabf3a3

…ive.

NSURLSession: optimized timout timer and fixed memory management

354df2a

NSURLSession: create dispatch queues using target and use lock instea…

e753c87

…d of queue for session identifier Creating stand-alone dispatch queues without a target is discouraged.

NSURLSession: call getTasks callbacks on delegate queue

719b384

Matches the documented behavior.

NSURLSession: use mutable data for transfer state body

54e0914

NSURLSession: improve protocol detection

2117a63

NSURLSession: fix libcurl debug output

ba0c1e1

NSURLSession: fix memory management in GSTransferState

792b027

NSURLSession: mark broken test as hopeful on Windows

6e1d53d

triplef force-pushed the nsurlsession-additions branch 8 times, most recently from 6d70c80 to 112e8fd Compare February 20, 2023 11:03

CI: add tools-make/tools-windows-msvc branch name to run name

1b5117f

triplef force-pushed the nsurlsession-additions branch 2 times, most recently from 764d4f1 to 6924590 Compare February 20, 2023 14:30

CI: fix MSVC builds when building dependencies

cb7e82e

triplef force-pushed the nsurlsession-additions branch from 6924590 to cb7e82e Compare February 20, 2023 15:00

triplef marked this pull request as ready for review February 20, 2023 16:06

triplef requested a review from rfm February 20, 2023 16:06

triplef mentioned this pull request Feb 22, 2023

Add libcurl build phase gnustep/tools-windows-msvc#15

Merged

rfm approved these changes Mar 11, 2023

View reviewed changes

triplef merged commit 8209793 into master Mar 11, 2023

triplef deleted the nsurlsession-additions branch March 11, 2023 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NSURLSession fixes and improvements #286

NSURLSession fixes and improvements #286

triplef commented Jan 13, 2023

rfm left a comment

rfm left a comment

triplef commented Jan 13, 2023

rfm left a comment

triplef commented Jan 13, 2023

rfm commented Jan 13, 2023 via email

triplef commented Jan 13, 2023

fredkiefer commented Jan 13, 2023

rfm commented Jan 13, 2023 via email

triplef commented Jan 13, 2023

triplef commented Jan 13, 2023

triplef commented Jan 16, 2023

rfm commented Jan 16, 2023 via email

triplef commented Jan 17, 2023

triplef commented Feb 20, 2023

triplef commented Feb 20, 2023

rfm left a comment

triplef commented Mar 11, 2023

NSURLSession fixes and improvements #286

NSURLSession fixes and improvements #286

Conversation

triplef commented Jan 13, 2023

rfm left a comment

Choose a reason for hiding this comment

rfm left a comment

Choose a reason for hiding this comment

triplef commented Jan 13, 2023

rfm left a comment

Choose a reason for hiding this comment

triplef commented Jan 13, 2023

rfm commented Jan 13, 2023 via email

triplef commented Jan 13, 2023

fredkiefer commented Jan 13, 2023

rfm commented Jan 13, 2023 via email

triplef commented Jan 13, 2023

triplef commented Jan 13, 2023

triplef commented Jan 16, 2023

rfm commented Jan 16, 2023 via email

triplef commented Jan 17, 2023

triplef commented Feb 20, 2023

triplef commented Feb 20, 2023

rfm left a comment

Choose a reason for hiding this comment

triplef commented Mar 11, 2023