#44 mllp client: read full response from the server

[cezio]

fix #44

[johnpaulett]

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]

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]

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

[asafsilman]

@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]

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

[johnpaulett]

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.

[johnpaulett]

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?