NSURLSession fixes and improvements

[triplef]

• 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]

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:

1. 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).
2. An unimplemented method returning an object conventionally returns the result of calling the -notImplemented: method.

[triplef]

1. 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.

2. 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]

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]

"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]

1. 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]

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]

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]

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]

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]

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]

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.

[triplef]

@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]

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]

Thanks @rfm!