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

NIORequestResponseWithIDHandler #157

Merged
merged 1 commit into from
Mar 22, 2023
Merged

NIORequestResponseWithIDHandler #157

merged 1 commit into from
Mar 22, 2023

Conversation

weissi
Copy link
Member

@weissi weissi commented Apr 21, 2022

Motivation:

NIORequestResponseHandler is great, for protocols where responses are in the same order as requests. Unfortunately, it doesn't work for protocols where responses may arrive out-of-order with regards to the order of the requests, such as NFS (#155).

Modifications:

  • Add NIORequestResponseWithIDHandler

Result:

Ability to easily handle protocols with out-of-order responses.

@weissi weissi marked this pull request as ready for review December 16, 2022 16:03
@weissi weissi requested a review from Lukasa December 16, 2022 16:04
Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Looks good but can we have a few tests?

Sources/NIOExtras/TaggedRequestResponseHandler.swift Outdated Show resolved Hide resolved
@glbrntt glbrntt added the 🆕 semver/minor Adds new public API. label Dec 16, 2022
@weissi
Copy link
Member Author

weissi commented Dec 16, 2022

@glbrntt thank you, added the tests. Added all the same ones as for the regular request/response handler plus a few extra tests that test out-of-order and receiving a response that doesn't match (wrong req id).

@weissi weissi requested a review from glbrntt December 16, 2022 17:40
Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Fab, you just need to regenerate the tests @weissi.

@weissi weissi enabled auto-merge (squash) December 19, 2022 11:06
///
/// `NIOTaggedRequestResponseHandler` does _not_ require that the `Response`s arrive on `Channel` in the same order as
/// the `Request`s were submitted. They are matched by their `requestID` property (from `NIORequestIdentifiable`).
public final class NIOTaggedRequestResponseHandler<Request: NIORequestIdentifiable,
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the naming of this channel handler because

  • We use two different words (Tagged and Identifiable) for the same underlying concept
  • A "Tag" usually doesn't uniquely identifies something e.g. the tag system for files on apple platforms

I think we should not use Tagged but use Identifiable, Identifier or ID for all names e.g. NIOIdentifiableRequestResponseHandler.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm "Identifiable" sounds weird. Maybe NIORequestResponseWithIDsHandler?

@weissi weissi requested a review from Lukasa March 21, 2023 12:08
@weissi weissi force-pushed the jw-tagged-rrh branch 2 times, most recently from eb37dfc to 6aba161 Compare March 22, 2023 11:48
@weissi weissi changed the title TaggedRequestResponseHandler NIORequestResponseWithIDHandler Mar 22, 2023
@weissi weissi requested a review from dnadoba March 22, 2023 11:49
@weissi weissi merged commit df28105 into apple:main Mar 22, 2023
@weissi weissi deleted the jw-tagged-rrh branch March 22, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants