Lingo

Race condition with ResultJoinStrategy causes premature result returns

Details

  • Type: Bug Bug
  • Status: Resolved Resolved
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.2.1
  • Fix Version/s: 1.3
  • Labels:
    None
  • Number of attachments :
    4

Description

There is a subtle race condition in JMSClientInterceptor:

ResultJoinHandler handler = createResultJoinHandler(methodInvocation, metadata);
// send request to topic
requestor.request(destination, requestMessage, handler, getMultipleResponseTimeout());
// race to here if the response comes back fast
RemoteInvocationResult result = handler.waitForResult();
return recreateRemoteInvocationResult(result);

due to a bug in the ResultJoinHandler methods:

public RemoteInvocationResult waitForResult() {
while (true) {
synchronized (lock) {
if (result != null)

{ return result; }

If the onMessage result could have been already called before we call waitForResult and so result could be != null but the unblockCallerThread could still return false.

I've attached a patch which fixes the problem. Few of the unit tests work for me because of outside dependencies so I'm not sure how to best test this issue.

  1. Text File
    lingo.patch
    27/Oct/06 11:39 AM
    5 kB
    Gray
  2. Text File
    lingo.patch
    27/Oct/06 12:30 AM
    5 kB
    Gray
  3. File
    ResultJoinHandler.java.diff
    26/Oct/06 9:08 PM
    2 kB
    Gray
  4. Text File
    ResultJoinHandler.java.patch
    13/Oct/06 12:20 PM
    1 kB
    Gray

Issue Links

relates to

Bug - A problem which impairs or prevents the functions of the product. LINGO-33 Multiple response timeout doesn't work

  • Critical - Crashes, loss of data, severe memory leak.
  • Resolved - A resolution has been taken, and it is awaiting verification by reporter. From here issues are either reopened, or are closed.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Gray added a comment -

I suspect that my patch may fix LINGO-33 as well.

Show
Gray added a comment - I suspect that my patch may fix LINGO-33 as well.
Gray made changes -
Field Original Value New Value
Link This issue depends upon LINGO-33 [ LINGO-33 ]
Gray made changes -
Link This issue depends upon LINGO-33 [ LINGO-33 ]
Gray made changes -
Link This issue is related to LINGO-33 [ LINGO-33 ]
Hide
Permalink
Gray added a comment -

Whoops. We may need to synchronize around the boolean. Not sure synchronizing on the lock is enough.

Show
Gray added a comment - Whoops. We may need to synchronize around the boolean. Not sure synchronizing on the lock is enough.
Hide
Permalink
Robert Newson added a comment -

I don't think the synchronization code in the class is being tested. If you comment out the notifyAll line, the test suite still passes. That's wrong, right?

Show
Robert Newson added a comment - I don't think the synchronization code in the class is being tested. If you comment out the notifyAll line, the test suite still passes. That's wrong, right?
Hide
Permalink
Gray added a comment -

Better version of the patch which reenables the timeout even if we didn't get all of the results. Also added a log message to track when this occurs.

Show
Gray added a comment - Better version of the patch which reenables the timeout even if we didn't get all of the results. Also added a log message to track when this occurs.
Gray made changes -
Attachment ResultJoinHandler.java.diff [ 23675 ]
Hide
Permalink
Gray added a comment -

Just uploaded a better patch which fixed some issues with the previous one – especially that it invalidated the timeout.

Show
Gray added a comment - Just uploaded a better patch which fixed some issues with the previous one – especially that it invalidated the timeout.
Hide
Permalink
Gray added a comment -

Yet another patch with a better solution. This fixes the original race condition but also adds an unblockAfterTimeout() method to the ResultJoinStrategy interface so the strategy can define the timeout and when to exit with no or partial results better.

Show
Gray added a comment - Yet another patch with a better solution. This fixes the original race condition but also adds an unblockAfterTimeout() method to the ResultJoinStrategy interface so the strategy can define the timeout and when to exit with no or partial results better.
Gray made changes -
Attachment lingo.patch [ 23676 ]
Hide
Permalink
james strachan added a comment -

Patch applied - many thanks!

Show
james strachan added a comment - Patch applied - many thanks!
james strachan made changes -
Fix Version/s 1.3 [ 12843 ]
Resolution Fixed [ 1 ]
Status Open [ 1 ] Resolved [ 5 ]
Hide
Permalink
Gray added a comment -

Okay. So this fixes a small problem with the previous patch. Can someone please remove all of the other patches. A couple are just wrong.

Show
Gray added a comment - Okay. So this fixes a small problem with the previous patch. Can someone please remove all of the other patches. A couple are just wrong.
Gray made changes -
Attachment lingo.patch [ 23686 ]
Hide
Permalink
james strachan added a comment -

Gray - its getting confusing with 4 patches on the go Could you create another patch off the current SVN trunk so I know which parts are changing after applying the previous patch?

Show
james strachan added a comment - Gray - its getting confusing with 4 patches on the go Could you create another patch off the current SVN trunk so I know which parts are changing after applying the previous patch?
Hide
Permalink
james strachan added a comment -

BTW as seen as 1.3 has been released, we should probably reopen another JIRA for turther patches

Show
james strachan added a comment - BTW as seen as 1.3 has been released, we should probably reopen another JIRA for turther patches
Hide
Permalink
Gray added a comment -

Sorry for the multiple patches. I would have deleted the old ones if I could. If it was going right into 1.3 then I would have reviewed it better.

Show
Gray added a comment - Sorry for the multiple patches. I would have deleted the old ones if I could. If it was going right into 1.3 then I would have reviewed it better.

People

  • Assignee:
    Unassigned
    Reporter:
    Gray
Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved: