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

seedlink basic client, get_info(): terminate properly #2996

Merged
merged 1 commit into from Mar 1, 2022

Conversation

megies
Copy link
Member

@megies megies commented Feb 28, 2022

What does this PR do?

Seems seedlink basic client in get_info() did not properly signal that everything was received
and ready to terminate. not sure why it worked before..

Why was it initiated? Any relevant Issues?

tries to fix #2995

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just add the "build_docs" tag to this PR.
  • If all tests including network modules (e.g. clients.fdsn) should be tested for the PR,
    just add the "test_network" tag to this PR.
  • All tests still pass.
  • Any new features or fixed regressions are covered via new tests.
  • Any new or changed features are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .
  • If the changes affect any plotting functions you have checked that the plots
    from all the CI builds look correct. Add the "upload_plots" tag so that plotting
    outputs are attached as artifacts.
  • Add the "ready for review" tag when you are ready for the PR to be reviewed.

@megies
Copy link
Member Author

megies commented Feb 28, 2022

Looks like return False in packet handler keeps the low-level client expecting more packets with data in them (here, metadata on available stations as xml plain text). So when a terminated/terminating (TYPE_SLINFT: Packet type is terminated info packet.) packet is received we should return True and then the low-level continuous packet reception loop is hitting a break statement and stopping.

Not sure how this worked before..

(seiscomp has had some major version jumps since this was first implemented, probably something changed in their seedlink server implementation and before the server was just stopping by itself and now is expecting a data request after we dont terminate properly)

@megies megies added the test_network tell github actions to also run network tests for this PR label Feb 28, 2022
@megies
Copy link
Member Author

megies commented Feb 28, 2022

@trichter ah, i should've probably added that network test label when opening the PR? 😬

@trichter
Copy link
Member

@trichter ah, i should've probably added that network test label when opening the PR?

Just push another empty commit to trigger the tests.

@trichter
Copy link
Member

That was a quick fix! Nice. It also reduces the spent time for network tests significantly 👍

@trichter
Copy link
Member

trichter commented Mar 1, 2022

Do you want to include a changelog or just merge?

seems it did not properly pass on the info that everything was received
and ready to terminate. not sure why it worked before..
@megies
Copy link
Member Author

megies commented Mar 1, 2022

Added changelog 👍

@megies megies merged commit 4301890 into master Mar 1, 2022
@megies megies deleted the fix_seedlink_get_info branch March 1, 2022 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.clients.seedlink test_network tell github actions to also run network tests for this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Seedlink client tests take very long, related error 'response: no data stream selector(s) accepted' logged
2 participants