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

Add native-mt support for CIO client #2021

Merged
merged 7 commits into from
Aug 17, 2020
Merged

Add native-mt support for CIO client #2021

merged 7 commits into from
Aug 17, 2020

Conversation

e5l
Copy link
Member

@e5l e5l commented Aug 14, 2020

No description provided.

@e5l e5l requested a review from Ololoshechkin August 14, 2020 07:57
@e5l e5l force-pushed the e5l/cio-common branch 2 times, most recently from 9ba9acf to 3963ef6 Compare August 17, 2020 07:14
Copy link
Contributor

@Ololoshechkin Ololoshechkin left a comment

Choose a reason for hiding this comment

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

Please, check all .api files: there seem to be remove and change operations in diff signalling that there might be breaking changes.

@e5l e5l force-pushed the e5l/cio-common branch 2 times, most recently from c9a1baa to c64977c Compare August 17, 2020 14:23
@e5l e5l changed the base branch from e5l/develop to master August 17, 2020 15:30
@e5l e5l changed the base branch from master to e5l/develop August 17, 2020 15:31
@e5l e5l changed the base branch from e5l/develop to master August 17, 2020 15:32
@e5l e5l merged commit 9785a7a into master Aug 17, 2020
@e5l e5l deleted the e5l/cio-common branch August 17, 2020 15:32
@vlsi
Copy link

vlsi commented Aug 19, 2020

This is wonderful, however, the following line in the release notes is hard to understand for those who have no idea what CIO is:

https://blog.jetbrains.com/ktor/2020/08/18/ktor-1-4-0-now-available/
Partial support for CIO (native-mt) which includes Sockets and HTTP*.

Please un-abbreviate the used acronyms at least once. Than you.

@dalewking
Copy link

Just upgraded to latest Ktor and the change in MockEngine to using ConcurrentList for storing request and response history is annoying. I have tests where I want to see how many times a particular call was made (I am testing some caching logic) using a method like this:

protected fun numberOfCallsMade(url: String)
        = mockEngine.requestHistory
            .map(HttpRequestData::url)
            .map(Url::toString)
            .filter { it == url }
            .count()

That now fails because ConcurrentList does not support an iterator.

@Stexxe
Copy link
Contributor

Stexxe commented Sep 17, 2020

@dalewking could you please file an issue in Youtrack with the full sample code attached?

@dalewking
Copy link

Don't see the point of sample code. The issue is simple. The object backing requestHistory and responseHistory were changed by this PR from MutableList to ConcurrentList and you can see right here that ConcurrentList does not support an iterator. So this is a regression in the features of requestHistory and responseHistory. That may be acceptable, but it isn't even documented.

@Stexxe
Copy link
Contributor

Stexxe commented Sep 18, 2020

@dalewking I've created a Youtrack issue about this problem.

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.

5 participants