Skip to content

Conversation

@knine79
Copy link

@knine79 knine79 commented Aug 21, 2025

Problem

The original code had a design issue where urlSession was created inside the events() function, but the cancel() method required it as a parameter. This caused the following problems:

  1. urlSession only existed within the function scope, making it impossible to access the correct session when calling cancel() from outside
  2. The cancel() method was public but couldn't receive the correct URLSession instance from external calls

Solution

  1. Store URLSession as instance variable: Added _urlSession Mutex variable to the DataTask class to manage the session object at the instance level
  2. Refactor cancel() method: Modified to be callable without parameters and use the stored session object for cancellation
  3. Clean up method signatures: Removed unnecessary urlSession parameters from handleSessionError, handleSessionResponse, parseMessages, and close methods
  4. Improve memory management: Set session to nil after cancellation to prevent memory leaks

Key Changes

  • Added _urlSession: Mutex<URLSession?> instance variable to DataTask class
  • Changed cancel() method signature from cancel(urlSession: URLSession) to cancel()
  • Removed urlSession parameter from all related methods
  • Added urlSession = nil in cancel() method for better memory management

Benefits

  1. Proper session management: The cancel() method can now accurately cancel the session that's actually in use
  2. API consistency: The cancel() method is more intuitive to call without parameters
  3. Memory safety: Clear lifecycle management of session objects
  4. Thread safety: Uses Mutex to handle concurrent access issues

Testing

  • All existing tests pass
  • Compilation successful

Breaking Changes

This change modifies the public API of the cancel() method. Previously it required a URLSession parameter, now it doesn't require any parameters. This is a breaking change but improves the API design significantly.

Related Issues

Fixes the design issue where external calls to cancel() couldn't properly cancel the URLSession due to scope limitations.

- Add URLSession as instance variable to DataTask class
- Remove urlSession parameter from cancel() method
- Update all related methods to use instance variable
- Improve memory management by setting urlSession to nil after cancel
- Fix issue where cancel() couldn't access the correct URLSession instance

This resolves the design issue where urlSession was created inside events()
function but cancel() method required it as a parameter, making it impossible
to properly cancel the session from external calls.
@knine79 knine79 requested a review from Recouse as a code owner August 21, 2025 01:21
@knine79 knine79 force-pushed the fix/dataTask-cancel-design branch 2 times, most recently from 4295467 to cba3aef Compare August 21, 2025 09:01
@knine79
Copy link
Author

knine79 commented Aug 21, 2025

@Recouse CI / Build on iOS 15 is failing because the ios-runtime cache isn’t being hit.
In another PR last month, the cache was hit, but not this time.
I’m not sure why. Could you investigate?

@xxZap
Copy link

xxZap commented Dec 4, 2025

Is there any news on this?

I updated from 0.1.4 to 0.1.5 and now I'm part of those who have a compilation issue because of the .cancel(urlSession:) param.

I don't really understand why this changed and why we are now forced to pass the URLSession since the used urlSession is an internal property inaccessible from the outside.
It really seems like a bug to me.

For the moment I'm going to lock to 0.1.4

@knine79
Copy link
Author

knine79 commented Dec 5, 2025

@xxZap Maybe @Recouse is very busy. 😅
As @Recouse mentioned in #36 (comment), cancel(urlSession:) has been changed to private last month.
It's been a while since I submitted the PR, and I haven't tested with the latest code, so I'm not sure if this is accurate, but
I think the intention is to save the DataTask returned from dataTask() and then use dataTask.cancel().
However, that code hasn't been released yet, so you'll probably need to reference the branch (main) in spm.
For reference, I switched to a different library. 😅😅😅

@knine79 knine79 closed this Dec 5, 2025
@xxZap
Copy link

xxZap commented Dec 11, 2025

@knine79
I understand your choice. In fact, for the moment I'm just locking to 0.1.4 since the 0.1.5 just does not compile and there's no way it can do it due to this new URLSession parameter.
I honestly don't want to (I can't) switch right now to a different library, I hope everything will be solved soon

@Recouse
Copy link
Owner

Recouse commented Dec 11, 2025

@xxZap could you try the main branch?

As previously mentioned, I decided to hide the cancel(urlSession:) method. You can achieve the same result by cancelling the parent Task, as shown in the example in the README.

If that doesn't work for you, please open an issue and describe your specific use case. I'll take a look and see what I can do.

I'm planning to make a new release soon. I've been busy recently and haven't had time to work on this project.

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.

3 participants