fix: continue monitoring if unhandled Exception is thrown #676

crystall-bitquill · 2023-10-10T22:19:19Z

Summary

Add additional logging for efm

Description

Adds additional logging in the MonitorImpl class for previously unhandled exceptions and for scenarios where monitoring has stopped but the startMonitoring method was called again.
Changes the monitoring loop to continue running if an unhandled exception is thrown.

Related to #675

Additional Reviewers

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

sergiyvamz · 2023-10-10T23:32:34Z

wrapper/src/main/java/software/amazon/jdbc/plugin/efm/MonitorImpl.java

    } finally {
      if (this.monitoringConn != null) {
        try {
          this.monitoringConn.close();
        } catch (final SQLException ex) {
          // ignore
+          LOGGER.warning(


Hmm I'm not sure this message adds extra clarity.

sergiyvamz · 2023-10-10T23:45:22Z

wrapper/src/main/java/software/amazon/jdbc/plugin/efm/MonitorImpl.java

@@ -94,6 +94,9 @@ public MonitorImpl(

  @Override
  public void startMonitoring(final MonitorConnectionContext context) {
+    if (this.stopped) {
+      LOGGER.warning(() -> Messages.get("MonitorImpl.monitorIsStopped"));


Can you add monitoring host name to the message?

davecramer · 2023-10-12T12:12:14Z

Do we have an issue to refer to that prompted this change ?

crystall-bitquill · 2023-10-12T17:37:31Z

Do we have an issue to refer to that prompted this change ?

Yes, #675. I've updated the PR description.

brunos-bq · 2023-10-16T16:56:23Z

wrapper/src/main/resources/aws_advanced_jdbc_wrapper_messages.properties

@@ -188,6 +188,9 @@ MonitorThreadContainer.emptyNodeKeys=Provided node keys are empty.

 # Monitor Impl
 MonitorImpl.contextNullWarning=Parameter 'context' should not be null.
+MonitorImpl.interruptedExceptionDuringMonitoring=Monitoring thread for node {0} was interrupted: {1}
+MonitorImpl.exceptionDuringMonitoring=Unhandled exception in monitoring thread for node {0}: {1}
+MonitorImpl.monitorIsStopped=Monitoring has already stopped for node {0}.


NIT: Monitor was or is already stopped?

brunos-bq · 2023-10-16T16:57:08Z

wrapper/src/main/java/software/amazon/jdbc/plugin/efm/MonitorImpl.java

+          () -> Messages.get(
+              "MonitorImpl.interruptedExceptionDuringMonitoring",
+              new Object[] {this.hostSpec.getHost(), intEx.getMessage()}));
+    } catch (final Exception ex) {


are we expecting any other kind of exception? (we did not catch it beforehand)

We're adding extra logging in case there was an exception we hadn't accounted for stopping this method and going undetected. This is because of the OOM error that was noticed after the newContexts queue had too many context objects added to it.

sergiyvamz · 2023-10-16T21:57:19Z

wrapper/src/main/java/software/amazon/jdbc/plugin/efm/MonitorImpl.java

+              new Object[] {this.hostSpec.getHost(), intEx.getMessage()}));
+    } catch (final Exception ex) {
+      // do nothing; exit thread
+      LOGGER.warning(


I think we need to change code so such unhandled exceptions to be logged but the monitoring thread/loop keeps running.

wrapper/src/main/java/software/amazon/jdbc/plugin/efm/MonitorImpl.java

davecramer · 2023-10-24T12:18:39Z

wrapper/src/main/java/software/amazon/jdbc/plugin/efm/MonitorImpl.java

-                this.activeContexts.add(monitorContext);
-                break;
-              }
+              synchronized (monitorContext) {


Does this really have to be synchronized? It is a local variable

It's a local variable but we add it to queues in MonitorImpl that runs in a separate thread. Monitoring thread may change an internal state of the context. The idea was to synchronize on it to avoid multi-threading collisions. It seems this intent isn't properly implemented here. Need to address it in a separate PR.

Why not fix it in this PR ?

After some investigation, it seems like the current implementation is working and a fix isn't required.

chore: add additional logging for efm

sergiyvamz reviewed Oct 10, 2023

View reviewed changes

crystall-bitquill force-pushed the issue-675 branch from 2cdb0f2 to 51659d2 Compare October 10, 2023 23:40

sergiyvamz reviewed Oct 10, 2023

View reviewed changes

crystall-bitquill force-pushed the issue-675 branch 2 times, most recently from 4ee88ef to 10ba058 Compare October 10, 2023 23:48

crystall-bitquill force-pushed the issue-675 branch from 10ba058 to 809b30c Compare October 12, 2023 22:04

brunos-bq reviewed Oct 16, 2023

View reviewed changes

crystall-bitquill force-pushed the issue-675 branch from 809b30c to f320b8b Compare October 16, 2023 19:54

sergiyvamz reviewed Oct 16, 2023

View reviewed changes

crystall-bitquill force-pushed the issue-675 branch from f320b8b to 0e6fd68 Compare October 17, 2023 19:33

crystall-bitquill changed the title ~~chore: add additional logging for efm~~ fix: continue monitoring unless InterruptedException is thrown Oct 17, 2023

crystall-bitquill force-pushed the issue-675 branch 2 times, most recently from eabc10f to 81530fe Compare October 21, 2023 01:59

davecramer reviewed Oct 23, 2023

View reviewed changes

wrapper/src/main/java/software/amazon/jdbc/plugin/efm/MonitorImpl.java Show resolved Hide resolved

davecramer reviewed Oct 23, 2023

View reviewed changes

wrapper/src/main/java/software/amazon/jdbc/plugin/efm/MonitorImpl.java Show resolved Hide resolved

crystall-bitquill force-pushed the issue-675 branch from 81530fe to e3da22e Compare October 24, 2023 01:52

davecramer reviewed Oct 24, 2023

View reviewed changes

crystall-bitquill force-pushed the issue-675 branch 2 times, most recently from a29d9c4 to 249a8b8 Compare October 24, 2023 19:36

crystall-bitquill changed the title ~~fix: continue monitoring unless InterruptedException is thrown~~ fix: continue monitoring if unhandled Exception is thrown Oct 24, 2023

crystall-bitquill force-pushed the issue-675 branch 2 times, most recently from ddc8a54 to 6b1e5c4 Compare October 24, 2023 19:40

sergiyvamz approved these changes Oct 25, 2023

View reviewed changes

crystall-bitquill force-pushed the issue-675 branch from 6b1e5c4 to e5dc854 Compare October 26, 2023 00:40

fix: continue monitoring if unhandled Exception is thrown

5a19f35

chore: add additional logging for efm

crystall-bitquill force-pushed the issue-675 branch from e5dc854 to 5a19f35 Compare October 26, 2023 02:11

crystall-bitquill merged commit 87ec172 into aws:main Oct 26, 2023
5 checks passed

crystall-bitquill deleted the issue-675 branch October 26, 2023 20:49

crystall-bitquill mentioned this pull request Oct 27, 2023

Memory Leak in EFM Plugin #675

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: continue monitoring if unhandled Exception is thrown #676

fix: continue monitoring if unhandled Exception is thrown #676

crystall-bitquill commented Oct 10, 2023 •

edited

Loading

sergiyvamz Oct 10, 2023

sergiyvamz Oct 10, 2023 •

edited

Loading

davecramer commented Oct 12, 2023

crystall-bitquill commented Oct 12, 2023

brunos-bq Oct 16, 2023

brunos-bq Oct 16, 2023

crystall-bitquill Oct 16, 2023

sergiyvamz Oct 16, 2023

davecramer Oct 24, 2023

sergiyvamz Oct 24, 2023

davecramer Oct 26, 2023

crystall-bitquill Oct 26, 2023

fix: continue monitoring if unhandled Exception is thrown #676

fix: continue monitoring if unhandled Exception is thrown #676

Conversation

crystall-bitquill commented Oct 10, 2023 • edited Loading

Summary

Description

Additional Reviewers

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Choose a reason for hiding this comment

sergiyvamz Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

davecramer commented Oct 12, 2023

crystall-bitquill commented Oct 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crystall-bitquill commented Oct 10, 2023 •

edited

Loading

sergiyvamz Oct 10, 2023 •

edited

Loading