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.

Sign up for GitHub

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

Jump to bottom

#44 mllp client: read full response from the server #49

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

#44 mllp client: read full response from the server #49

wants to merge 3 commits into from

Conversation

cezio
Copy link

@cezio cezio commented Jun 10, 2022

fix #44

@johnpaulett
Copy link
Owner

Thanks for the contribution -- the client was mainly written to provide simple testing via the mllp_send command, so it could use some work to be more "production-ready".

  • The addition of duration feels odd to me -- it means even a fast responding server that sends a single response will take a minimum of 3 seconds until the client returns the result, right? Not fully thought thru, but should the receiving of the messages instead become more aware of the LLP protocol and receive until it receives the end of message?
  • timeout seems reasonable. 👍
  • We'll want test coverage for the changes (but I think the duration potentially needs to be reworked)
  • I'd suggest removing the logging.debug -- I get it was probably useful when developing this PR, but don't think it adds much in real world running, since the received data will just be returned.

@cezio
johnpaulett#44 client code updated
6f2eae5 
* mllp_client code updated to react on timeouts from socket, use of argparse instead of obsolete optparse
* tests updated to follow new features and convetions
@cezio
johnpaulett#44 - stop waiting for recv() when MLLP end bytes already …
b1e40ac 
…arrived
@cezio
Copy link
Author

cezio commented Aug 16, 2022

Thanks for the contribution -- the client was mainly written to provide simple testing via the mllp_send command, so it could use some work to be more "production-ready".

In most cases I use MLLPClient class directly, but mllp_send command is an invaluable friend in some rescue-the-world scenarios.

  • The addition of duration feels odd to me -- it means even a fast responding server that sends a single response will take a minimum of 3 seconds until the client returns the result, right? Not fully thought thru, but should the receiving of the messages instead become more aware of the LLP protocol and receive until it receives the end of message?

It's application-level deadline for the ack receiving operation. I had to split this from socket-level timeout because of the way the client is used inside my code. But yes, you're right it would wait duration time even if the ACK was already received in full. I've added a simple check for this case.

  • timeout seems reasonable. +1
  • We'll want test coverage for the changes (but I think the duration potentially needs to be reworked)

Yep, tests from test_client.py were failing. Sorry, my bad. I didn't check them in the first run.

I've updated tests to follow changes in client code and added a test for end-of-message bytes check.

  • I'd suggest removing the logging.debug -- I get it was probably useful when developing this PR, but don't think it adds much in real world running, since the received data will just be returned.

If you use mllp_client command, then it may looks redundant. However, when it's below few layers of code, you can control this with a proper logging setup of the application.

I've added a condition to set appropriate logging levels depending on verbose flag when mllp_send is called. Would that work for you?

@NiklasMM
Copy link

I think we might be experiencing this issue as well. Any chance to get this PR finished? I'm happy to help.

@asafsilman
Copy link

@johnpaulett Any update on this PR, we are also waiting for this feature to be implemented as it would be incredibly handy for our testing.

@LukasKlement
Copy link

Could this PR be merged or are there any blockers left?

johnpaulett
johnpaulett requested changes Feb 2, 2023
View reviewed changes
Copy link
Owner

@johnpaulett johnpaulett left a comment

Choose a reason for hiding this comment

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

My apologies for the delay. Here is what I think this needs to get merged

  1. I do not understand the need for the duration. Can we simply check for the end of LLP message EB + CR with a timeout in a while True? The duration means that every send (even to servers that send back in a single TCP message) will take a minimum of 3 seconds by default. Unless a strong case for the duration on top of the end of LLP + timeout can be made, I believe we should remove the duration aspect.
  2. Please remove the logging here -- it doesn't add any benefit: we're just logging the returned value which the caller of the MLLPClient could do if it wishes and we already get to stdout when called via mllp_send

I'm not opposed to switching to argparse, but in the future would suggest separate PRs as the code improvement of upgrading to argparse is unrelated to issue of #44.

hl7/client.py
Comment on lines +311 to +314
try:
mllp_send(sys.argv)
except CLIException as err:
sys.exit(err.exit_code)
Copy link
Owner

Choose a reason for hiding this comment

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

This is going to hide the actual stack trace of what went wrong, isn't it. E.g. it won't be clear if it was a TimeoutError or a parsing exception?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnpaulett johnpaulett johnpaulett requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MLLP client doesn't receive full ack response when expected
5 participants
@cezio @johnpaulett @NiklasMM @asafsilman @LukasKlement