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

Gemini -(Kumo) Task created in a session that has been invalidated #23

Merged
merged 8 commits into from Mar 31, 2023

Conversation

gnkeshavamurthy
Copy link
Collaborator

DESCRIPTION

Fixing Task created in a session that has been invalidated crash

CHANGE

  • Changed Service from class to actor
  • Updated perform() with async & await

@gnkeshavamurthy gnkeshavamurthy force-pushed the refactor/GS-2688_invalid_session branch 3 times, most recently from 9d08f03 to 728fc3d Compare March 24, 2023 13:41
@gnkeshavamurthy gnkeshavamurthy force-pushed the refactor/GS-2688_invalid_session branch from 728fc3d to 3bae535 Compare March 24, 2023 13:52
Copy link
Collaborator

@powerje powerje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had some questions about the changes

Sources/Kumo/Blobs/BlobCache.swift Outdated Show resolved Hide resolved
Tests/KumoTests/Fixtures/Common/NetworkTest.swift Outdated Show resolved Hide resolved
@@ -5,47 +5,47 @@ import XCTest
class GetTests: NetworkTest {

func testSuccessfulGetRequestEmittingVoid() {
successfulTest(of: service.perform(HTTP.Request.get("get")))
successfulTest(of: performs(HTTP.Request.get("get")))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still calling a function on the service somehow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I have added a method to call service.perform as we have to use Task {} for this call

         Task {
                    do {
                        let result: T = try await service.perform(request)
                        promise(.success(result))
                    } catch let error {
                        promise(.failure(error))
                    }
                }

so I'm using that common method in tests

@powerje
Copy link
Collaborator

powerje commented Mar 24, 2023

Running the tests ($ swift test) there are some warnings:

Service.swift:285:10: note: calls to instance method 'downloadResultToURL(url:response:)' from outside of its actor context are implicitly asynchronous
    func downloadResultToURL(url: URL?, response: URLResponse?) throws -> URL {
...
 warning: non-sendable type 'OperationQueue?' passed in call to nonisolated initializer 'init(baseURL:runsInBackground:logger:delegateQueue:configuration:)' cannot cross actor boundary
    let service = Service(baseURL: URL(string: "https://httpbin.org")!, logger: TestLogger())
...
Swift.Optional:1:21: note: generic enum 'Optional' does not conform to the 'Sendable' protocol
@frozen public enum Optional<Wrapped> : ExpressibleByNilLiteral {

I think we should address these?

@gnkeshavamurthy
Copy link
Collaborator Author

Running the tests ($ swift test) there are some warnings:

Service.swift:285:10: note: calls to instance method 'downloadResultToURL(url:response:)' from outside of its actor context are implicitly asynchronous
    func downloadResultToURL(url: URL?, response: URLResponse?) throws -> URL {
...
 warning: non-sendable type 'OperationQueue?' passed in call to nonisolated initializer 'init(baseURL:runsInBackground:logger:delegateQueue:configuration:)' cannot cross actor boundary
    let service = Service(baseURL: URL(string: "https://httpbin.org")!, logger: TestLogger())
...
Swift.Optional:1:21: note: generic enum 'Optional' does not conform to the 'Sendable' protocol
@frozen public enum Optional<Wrapped> : ExpressibleByNilLiteral {

I think we should address these?

Fixed this warning

gnkeshavamurthy and others added 2 commits March 28, 2023 19:17
Updated swift version and removed `RxSwift` dependency from podspec.
`RxSwift` requirement was removed in the 2.0.0 update but stuck around
the podspec erroneously.
@powerje powerje merged commit a6d149b into master Mar 31, 2023
4 checks passed
@powerje powerje deleted the refactor/GS-2688_invalid_session branch March 31, 2023 13:39
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.

None yet

2 participants