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

CAMEL-20121 reconnect SMPP session after receiving Unbound #12046

Merged
merged 2 commits into from
Jan 17, 2024

Conversation

der-ambi
Copy link
Contributor

Description

The SMPP peer I am using Camel against sends an Unbind after an inactivity period of 60 seconds on the connection. This means that the logical connection has been terminated and (as I understand the SMPP spec) the peer that received the Unbind is supposed to terminate the connection as the session cannot be recovered.

SmppProducer/SmppConsumer don't react on this event and trying to send a SMS after receiving the Unbind ends up in exceptions (org.apache.camel.component.smpp.SmppException: java.io.IOException: Cannot submitShortMessage while session 0f46dc52 in state UNBOUND)

This PR adds code to catch the UNBOUND event in both SmppProducer/SmppConsumer and also closes the previous test gap of the internalSessionStateListener being uncovered in tests.

Target

  • I checked that the commit is targeting the correct branch (note that Camel 3 uses camel-3.x, whereas Camel 4 uses the main branch)

Tracking

  • If this is a large change, bug fix, or code improvement, I checked there is a JIRA issue filed for the change (usually before you start working on it).

Apache Camel coding standards and style

  • I checked that each commit in the pull request has a meaningful subject line and body.
  • I have run mvn clean install -DskipTests locally and I have committed all auto-generated changes

Copy link
Contributor

🌟 Thank you for your contribution to the Apache Camel project! 🌟

🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

  • First-time contributors require MANUAL approval for the GitHub Actions to run

  • You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot.

  • You can label PRs using build-all, build-dependents, skip-tests and test-dependents to fine-tune the checks executed by this PR.

  • Build and test logs are available in the Summary page. Only Apache Camel committers have access to the summary.

  • ⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

@der-ambi
Copy link
Contributor Author

Hold on..... I changed the handling of unbound to reconnect instead of just closing the session as last-minute change, but didn't properly check if it really resolves my scenario.

Looks like just closing the session and creating a fresh one next time an Exchange comes in might be a cleaner solution.

How do you think it should behave? Directly reconnect or wait for the next exchange to send before establishing a new connection?

@davsclaus
Copy link
Contributor

Yeah I think the producer should wait for next message

@der-ambi
Copy link
Contributor Author

der-ambi commented Nov 21, 2023

Seems like closing the session in the StateListener can trigger a bug in jSMPP: opentelecoms-org/jsmpp#193

@davsclaus
Copy link
Contributor

@der-ambi are you able to work on the last bit

@der-ambi
Copy link
Contributor Author

der-ambi commented Dec 4, 2023

@davsclaus as jsmpp will deadlock when closing the session from within the Session State Listener I have to either wait for a fix in jsmpp or introduce a new Background Task that will close the session asynchronously. But that will also require more code for proper synchronisation.
What would you prefer? My preference would be to get jsmpp fixed instead of adding additional complexity on the camel side to work-around the problem in jsmpp.

@davsclaus
Copy link
Contributor

@davsclaus as jsmpp will deadlock when closing the session from within the Session State Listener I have to either wait for a fix in jsmpp or introduce a new Background Task that will close the session asynchronously. But that will also require more code for proper synchronisation. What would you prefer? My preference would be to get jsmpp fixed instead of adding additional complexity on the camel side to work-around the problem in jsmpp.

Yes its of course best to have it fixed if possible in the underlying library. Is that team working on a fix ?

@der-ambi
Copy link
Contributor Author

der-ambi commented Dec 6, 2023

I am trying to revive the discussion by proposing some patches.. let's see what happens

@der-ambi
Copy link
Contributor Author

Upstream jsmpp has accepted my patches and released a fixed version 3.0.1

@davsclaus davsclaus merged commit 6a8d992 into apache:main Jan 17, 2024
3 checks passed
ryan-highley pushed a commit to ryan-highley/camel that referenced this pull request Apr 8, 2024
)

* update version of jsmpp to 3.0.1

Signed-off-by: Christian Ambach <[email protected]>

* CAMEL-20121 reconnect SMPP session after receiving Unbind

Signed-off-by: Christian Ambach <[email protected]>

---------

Signed-off-by: Christian Ambach <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants