Submission Svc: Timeout handling

[stevesap]

Checklist

• Test your changes as thoroughly as possible before you commit them. Preferably, automate your test by unit/integration tests.
• Make sure mvn install runs for the whole project and, if you touched any code in the respective service, submission and distribution service can be run with spring-boot:run

Description

This PR is a proposal taking into account the analyses in #423

Introduces two submission service related timeouts:

FeignClient timeouts

feign:
  client:
    config:
      default:
        connect-timeout: 5000
        read-timeout: 5000

• These timeouts are required in order to prevent blocked submission service worker threads in the case where the cwa-verification-server does not respond in timely fashion, likely due to a flood of invalid TAN requests.
• The above values are similar to those in cwa-verification-portal and considerably less than the 60 second default value.
• A FeignClient timeout results in Feign.RetyrableExeption which gets handed over to DeferredResult and then immediately interrupts the synchronous endpoint execution, resulting in a HTTP 500 Internal Server Error.

Transaction timeout

spring:
  transaction:
    default-timeout: 20

• This timeout is required in order to prevent blocked submission service worker threads in the case where the database does not respond in a timely fashion. Note that this is not as likely to happen as much as a cwa-verification-server slow response, however if the database does not respond for whatever reason, the thread will continue waiting for it indefinitely (This was tested this introducing a large delay into the SQL INSERT INTO statement)
• The above value (in seconds) was chosen as the maximum amount of time that the diagnosis key insertion transaction should reasonably be concluded by.
• A transaction timeout results in org.springframework.transaction.TransactionTimedOutException which gets handed over to DeferredResult and then immediately interrupts the synchronous endpoint execution, resulting in a HTTP 500 Internal Server Error.

Note: There is no need to set a timeout on DeferredResult or the submission service endpoint itself since due to the submission endpoint currently being implemented synchronously, a timeout on DeferredResult has no effect. As noted, the above 2 timeouts will be handed over to DeferredResult which would interrupt the synchronous endpoint execution immediately.

[RomanNess]

I have two remarks that might not carry weight in the specific case.

1. spring.transaction.default-timeout is a global setting that will apply a timeout to all functions annotated with @Transactional. It will also trigger for service functions annotated with '@Transactional` that do not call the DB at all.

   A local timeout on function level can be configured with @Transactional(timeout = 20, propagation = Propagation.REQUIRES_NEW) where Propagation.REQUIRES_NEW makes sure that a new transaction with the timeout is used even if a transaction without timeout is already active.

2. To me it seems a bit odd that HTTP 500 is returned for both timeouts in FeignClient and timeouts against the DB. In order to distinguish these timeouts that indicate load issues in other components from actual server errors in cwa-server, I would have used something like HTTP 504 Gateway Timeout.

Once again, I don't think that this will be an issue for cwa-server in production. These are just two thoughts that came into my mind, reading this pull request.

[stevesap]

1. This is a good point. However, the function in question is the only one in the submission service which uses @Transactional annotation. The main reason I went with the global application configuration setting here is that @Transactional's timeout parameter only accepts constants and I would prefer to use an application configuration property here in case this value needs to be adjusted quickly in production.
   By any chance that additional functions require @Transactional annotation down the road, we can
   look at the somewhat more invasive change of using TransactionTemplate to apply a different timeout to each, using application configuration properties.

2. I agree that HTTP 504 is more semantically correct here, however I'm not sure what additional value that would bring. If the mobile clients used this information somehow, that would make more sense. It would indeed expose externally that something is wrong with our verification-server or database, but this is something our Ops team would already be able to identify internally since these very specific exceptions (Feign.RetyrableExeption and org.springframework.transaction.TransactionTimedOutException) are getting logged.

[RomanNess]

1. The flexibility to change the timeout parameter without recompilation is a good point.

2. The additional value comes when the mobile client reacts to different 5xx status codes. For example the client could display Please try again later for 504 (since Gateway timeouts are typically caused by too much load on the system) and something like Unexpected error for 500.

   Also, it might be easier to group exceptions by status code if you don't have a dedicated Ops team and you have to do the Ops for your own code. 🙂

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
0.0% Duplication