Skip to content

Ensure IMDServer sends EOF to client DURING test#5443

Merged
orbeckst merged 5 commits into
MDAnalysis:developfrom
ljwoods2:patch-2
Jul 20, 2026
Merged

Ensure IMDServer sends EOF to client DURING test#5443
orbeckst merged 5 commits into
MDAnalysis:developfrom
ljwoods2:patch-2

Conversation

@ljwoods2

@ljwoods2 ljwoods2 commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Fixes #5442

Changes made in this Pull Request:

  • IMDServer now calls disconnect after sending frames so that the client does not rely on the (now 600s) timeout to see the end of the stream

LLM / AI generated code disclosure

LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: no

PR Checklist

  • Issue raised/referenced?
  • Tests updated/added?
  • Documentation updated/added?
  • package/CHANGELOG file updated?
  • Is your name in package/AUTHORS? (If it is not, add it!)
  • LLM/AI disclosure was updated.

Developers Certificate of Origin

I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.

@read-the-docs-community

read-the-docs-community Bot commented Jul 19, 2026

Copy link
Copy Markdown

@ljwoods2
ljwoods2 marked this pull request as ready for review July 19, 2026 21:31

@orbeckst orbeckst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The test_imd.py::test_imd_stream_empty test still fails:

FAILED testsuite/MDAnalysisTests/coordinates/test_imd.py::test_imd_stream_empty - Failed: Timeout (>200.0s) from pytest-timeout.

@orbeckst orbeckst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks like that some of the test cases never establish the connection and then they fail with


    def disconnect(self):
>       self.conn.shutdown(socket.SHUT_RD)
        ^^^^^^^^^^^^^^^^^^
E       AttributeError: 'NoneType' object has no attribute 'shutdown'

@orbeckst orbeckst self-assigned this Jul 20, 2026
@orbeckst

Copy link
Copy Markdown
Member

Please black testsuite/MDAnalysisTests/coordinates/test_imd.py to make the linter happy (needs black v24).

@IAlibay IAlibay left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, just the one question, but otherwise this seems like a reasonable fix.

# Send the rest of the frames- small enough to all fit in socket itself
ref.server.send_frames(1, 5)
# Send EOF so the client does not rely on timeout to mark the end of the stream
ref.server.disconnect()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just checking, is this because backward compatible or should we be adding a minimum pin?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This should be backwards compatible. The disconnect call exists and is identical in the pinned version (0.2.2)

https://github.com/Becksteinlab/imdclient/blob/07069e1ff08d1488936c61e52f922116ecddf210/imdclient/tests/server.py#L204

@orbeckst orbeckst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We'll also release a 0.2.5 imdclient but that's not required for this fix to work. LGTM. Thanks @ljwoods2 !

@orbeckst
orbeckst merged commit 830e2af into MDAnalysis:develop Jul 20, 2026
10 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

some IMDReader streaming tests time out with imdclient 0.2.4

3 participants