Uploaded image for project: 'James Server'
  1. James Server
  2. JAMES-482

incorrect state on duplicate mail (LinearProcessor)

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: 2.2.0, 2.3.0
    • Fix Version/s: 2.3.0
    • Labels:
      None

      Description

      There is a bug in the LinearProcessor. When a Mail is duplicted because a matcher has matched only some of the recipients, the state of the new mail for the not matched recipients is not set correctly.

      Imagine the following scenario: There is a processor which is not the root processor. In this processor there is a matcher (let's call ist MatcherOne) which matches only some on the recipients of a Mail. This is followed by a matcher (let's call it MatcherTwo) which matches some or all of the recipients which were not matched by MatcherOne. The Mailet for MatcherTwo does not set the Mail's state. This is followed by more matcher-mailet-pairs.

      The following ought to happen: When MatcherOne matches some of the recipients, the Mail gets duplicated. The orginal instance is sent to the mailet of MatcherOne. The duplicate for the not matched recipients is handed to the next matcher: MatcherTwo. Let's say that MatcherTwo matches all remaining recipients. Than the mailet of MatcherTwo is processed. Since this mailet does not change the state, the next matcher in the current processor should be used next.

      What really happens because of the bug: When MatcherOne matches some of the recipients, the Mail gets duplicated. The orginal instance is processed correctly. The duplicate for the not matched recipients has the incorrect state "root". Nevertheless, it is handed to the next matcher of the current processor: MatcherTwo. Let's say that MatcherTwo matches all remaining recipients. Then, the mailet of MatcherTwo is processed. This mailet does not change the state, but the Mail is then treated as if that mailet had changed the state to "root". Thus the whole processing chain starts over again. This does not cause an endless loop, becauce when the Mail will reach the processor where all this happened again, MatcherOne will match all remaining recipients, so that no further duplication of the Mail will take place. The Mail will than complete the processing chain. But some matchers and mailets from before this point will have been called twice. In most cases, this might remain unnoticed, but if those mailets do stuff like adding footers, those footers are added twice.

      Possible fixes:
      Add
      setState(mail.getState());
      to the constructor MailImpl(Mail,String) or add
      notMail.setState(mail.getState());
      to the service method of LinearProcessor.
      I don't know which fix might be preferrable.

        Activity

        Hide
        brainlounge Bernd Fondermann added a comment -

        it would be great to have a unit test reproducing this.

        Show
        brainlounge Bernd Fondermann added a comment - it would be great to have a unit test reproducing this.
        Hide
        norman Norman Maurer added a comment -

        I will have a look but not sure how to write this

        Show
        norman Norman Maurer added a comment - I will have a look but not sure how to write this
        Hide
        bago Stefano Bagnara added a comment -

        Look at the LinearProcessorTest I wrote to prove JAMES-421.

        I'm also not sure about the best of the 2 solutions. Fixing the "duplicator" constructor seems to me the cleaner way, but this could change how things works in other places of the code. We should probably review all the calls to that constructor and understand wether we need or not to keep the state.

        Show
        bago Stefano Bagnara added a comment - Look at the LinearProcessorTest I wrote to prove JAMES-421 . I'm also not sure about the best of the 2 solutions. Fixing the "duplicator" constructor seems to me the cleaner way, but this could change how things works in other places of the code. We should probably review all the calls to that constructor and understand wether we need or not to keep the state.
        Hide
        brainlounge Bernd Fondermann added a comment -

        Maybe LinearProcessorTest could be of use for that? Seems like it does set up mailets and works with them.

        Show
        brainlounge Bernd Fondermann added a comment - Maybe LinearProcessorTest could be of use for that? Seems like it does set up mailets and works with them.
        Hide
        norman Norman Maurer added a comment -

        I will have a look. Maybe i can write a unit test.

        Show
        norman Norman Maurer added a comment - I will have a look. Maybe i can write a unit test.
        Hide
        bago Stefano Bagnara added a comment -

        "t" is the LinearProcessor in that test.
        It could be changed so that most of setUp and tearDown code is moved directly in the testCopyOnWrite so that further tests with different LinearProcessors could be written in the same class.

        Show
        bago Stefano Bagnara added a comment - "t" is the LinearProcessor in that test. It could be changed so that most of setUp and tearDown code is moved directly in the testCopyOnWrite so that further tests with different LinearProcessors could be written in the same class.
        Hide
        bago Stefano Bagnara added a comment -

        Increased the priority to critical: this change the way mail processing is done to a different way from what everyone expect.

        Show
        bago Stefano Bagnara added a comment - Increased the priority to critical: this change the way mail processing is done to a different way from what everyone expect.
        Hide
        bago Stefano Bagnara added a comment -

        Added a test and applied the fix.
        I decided to change only the linearprocessor because when other code clone a mail doesn't make sense to keep the state.
        It's left to the mailet code to set a given state before calling sendMail.

        Show
        bago Stefano Bagnara added a comment - Added a test and applied the fix. I decided to change only the linearprocessor because when other code clone a mail doesn't make sense to keep the state. It's left to the mailet code to set a given state before calling sendMail.
        Hide
        danny@apache.org Danny Angus added a comment -

        Closing issue fixed in released version.

        Show
        danny@apache.org Danny Angus added a comment - Closing issue fixed in released version.

          People

          • Assignee:
            bago Stefano Bagnara
            Reporter:
            norman Norman Maurer
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development