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

fix: loadedMessage is not cleared after message(s) handled #102

Closed
jtarvainen opened this issue Aug 21, 2024 · 10 comments · Fixed by #113
Closed

fix: loadedMessage is not cleared after message(s) handled #102

jtarvainen opened this issue Aug 21, 2024 · 10 comments · Fixed by #113
Assignees
Labels
bug Something isn't working help wanted Extra attention is needed question Further information is requested

Comments

@jtarvainen
Copy link

jtarvainen commented Aug 21, 2024

Environment

  • HL7 Client version: 2.3.1
  • HL7 Server version: 2.4.1

Questions:

  • Are you using the Node HL7 Server package as your HL7 Server to connect to or a 3rd party system?

Yes

Expected Behavior

The Inbound instance's "data" event handler should clear loadedMessage after handling a complete message (or batch of messages). Otherwise the variable just keeps concatenating each received message in perpetuity.

Actual Behavior

loadedMessage is never cleared, causing each previously received message to be re-handled upon reception of each new message.

The fix is to add the following after row 220 in src/server/inbound.ts:

loadedMessage = '';

Steps to reproduce the bug

Create a unit test where:

  1. Client initiates a connection to the server
  2. Client sends a message to the server and waits for a response (connection is kept open!)
  3. Client sends a second message to server and waits for a response
  4. Client closes the connection to the server

Assert that in step #3 the previously sent message is re-handled on the server in addition to the new message and two acknowledgments are sent to the client instead of one.

@jtarvainen jtarvainen added the bug Something isn't working label Aug 21, 2024
@Bugs5382 Bugs5382 self-assigned this Aug 24, 2024
@Bugs5382
Copy link
Owner

@jtarvainen My coding days are tomorrow. I will work on this and release the other feature for AE.

@Bugs5382 Bugs5382 added the question Further information is requested label Aug 25, 2024
@Bugs5382
Copy link
Owner

So doing so breaks up a unit test for very large messages, like getting PDF data.

let loadedMessage: string = ''

Already has clearing of the loadedMessage on a TCP connect. I might have to write a unit test out for this one to test out this issue.

@Bugs5382
Copy link
Owner

This could be a bug in the node-hl7-client as well.

@jtarvainen
Copy link
Author

The client device we use keeps the TCP connection alive between messages, so we can't rely on the clear-on-connect feature.

@Bugs5382
Copy link
Owner

Bugs5382 commented Sep 2, 2024

@jtarvainen Just so you know, I am trying to nail this down. Some major testing I been writing locally.

@jtarvainen
Copy link
Author

jtarvainen commented Sep 19, 2024

@Bugs5382 Any update on this? I haven't investigated the code further, but to me it sounds like it should be OK to clear loadedMessage after the current message (or batch of messages) has been fully processed, even if it contains PDF data?

@Bugs5382
Copy link
Owner

@jtarvainen I am still working. I tried that, and my unit tests failed hard. Maybe I am missing something.

@Bugs5382
Copy link
Owner

Bugs5382 commented Nov 1, 2024

This fix is going to be out soon. Thanks @jtarvainen for your patience.

@Bugs5382 Bugs5382 added the help wanted Extra attention is needed label Nov 1, 2024
@Bugs5382
Copy link
Owner

Bugs5382 commented Nov 1, 2024

@jtarvainen This has been release 3.0.1-develop

@jtarvainen
Copy link
Author

That's great, thank you!

@Bugs5382 Bugs5382 mentioned this issue Nov 16, 2024
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants