-
Notifications
You must be signed in to change notification settings - Fork 23
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 message reader and writer #44
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
alovak
force-pushed
the
add-message-reader-writer
branch
from
May 16, 2023 15:14
3ab3173
to
ca91feb
Compare
alovak
force-pushed
the
add-message-reader-writer
branch
from
May 16, 2023 15:40
ca91feb
to
7de4187
Compare
adamdecaf
approved these changes
May 17, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice improvement!
So much easier to comprehend! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
In some cases, we may need to reply to the incoming message and copy some fields of the request message header into the reply header. Currently, the
iso8583-connection
package supports message headers (we call it network headers) with the message length only. It also does not link message to the header, so there is no way to use data of the incoming message header in the response.Summary of the PR
Introduce the message reader and writer interfaces to support implementations that will pack/unpack and write/read messages to/from the network connection letting them use the mechanism of linking headers with requests and responses. We also moved message packing and unpacking right into message writer/reader and write
*iso8583.Message
into the channels instead of raw data ([]byte
).Changes
MessageReader
andMessageWriter
which if set replaceMessageLengthReader
andMessageLengthWriter
*iso8583.Message
to thereadResponseCh
channel instead of passing raw data ([]byte
)Send
MessageReader
returnsnil
message andnil
error (same as is suggested here)ErrUnpack
into idiomaticUnpackError
If you don't work directly with
ErrUnpack
, then this is a backward compatible change. but if you do, you should rename the type. I also think we should removeMessageLengthReader
andMessageLengthWriter
in one of the next releases.Example of the new use case
Here is an example of how you may use a message reader and writer to link request and response headers:
First, we should create a message reader and writer. They will read/write the message header, read/write the message itself and unpack/pack it. In addition, as shown in the example, you can store request headers in some storage and then use data from the header for the reply:
Second, set
MessageReader
andMessageWriter
instead ofMessageLengthReader
andMessageLengthWriter
:Performance Impact
Moving message packing from
Send
message to the message writer could negatively impact the performance of the package. We have (fixed) and run benchmarks to see that new results a not critically worse than they were before. Here are the new results from running benchmarks on the same machine:It takes 2.7ms to 100 times: send message (client), receive message (server), respond to the message (server), receive response (client).