Repository navigation
Write message tracking events in order on one thread - #638
Merged
Merged
Conversation
JakeHuneau
force-pushed
the
fix/tracking-write-order
branch
from
November 2, 2026 14:22
82e53e3 to
152cf94
Compare
Each tracking event was written on a new thread of its own so that processing a message did not wait on the database. DbTrackingModule.persist is synchronized, so when a write was slow the threads behind it queued on the lock and took it in no particular order. Under load a message's events could be written out of order and an earlier state overwrite a later one, which is what issue OpenAS2#517 reports: a message that had received its MDN left recorded with the state of "sending started". BaseMsgTrackingModule now hands each event to a single-thread executor of its own, so the writes stay off the processing thread and happen in the order the events were handled. The thread is a daemon so it never keeps the JVM alive. The tracking module is an ActiveModule, so the shutdown drain added in OpenAS2#625 already calls its awaitIdle with the shared deadline after the pollers have finished their sends. awaitIdle now waits for the events queued so far to be written, so the record of a message sent just before shutting down is not lost. DbTrackingModule.stop stops the writer and start gives a restarted module a new one; an event handled after the writer has stopped is written directly rather than dropped. The maintainer's comment on OpenAS2#517 mentions moving tracking onto a queue in 5.0; this is the minimal form of that and should fit into it. TrackingWriteOrderTest writes 200 events through a tracker whose synchronized persist is slow on the first write. With the previous thread per event all 200 were written but out of order in three runs out of three; it now writes them in order. The other tests cover not blocking the caller, awaitIdle and its timeout, writing directly after stopping, restarting, and shutdown waiting for queued events.
JakeHuneau
force-pushed
the
fix/tracking-write-order
branch
from
November 2, 2026 15:18
152cf94 to
662a3f2
Compare
uhurusurfa
reviewed
Nov 2, 2026
| return false; | ||
| } catch (ExecutionException e) { | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
Shouldn't this just call:
writer.shutdown();
writer.awaitTermination(timeoutMillis, TimeUnit.MILLISECONDS);
Collaborator
Author
There was a problem hiding this comment.
Agreed — changed to writer.shutdown(); writer.awaitTermination(...) in 0bafc65. I had used a marker so the writer stayed open, but handle() already writes an event directly once the writer has stopped, so events from receivers that are still running during shutdown are not lost either way. Added a test for that case.
As suggested in review, awaitIdle now shuts the writer down and waits for it to terminate, rather than queueing a marker task and waiting for that. It is only called while shutting down, and an event handled after the writer has stopped, for example by a receiver that is still running until the active modules are stopped, is already written directly by handle(), so nothing is lost and the code is simpler. A new test covers an event handled after awaitIdle being written.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #517.
Problem
BaseMsgTrackingModule.handlewrites each tracking event on a new thread of its own, so processing a message doesn't wait on the database.DbTrackingModule.persistissynchronized. So when one write is slow, the threads behind it queue on the lock and take it in no particular order.Under load, a message's events can be written out of order and an earlier state can overwrite a later one. That's what #517 reports: a message that has received its MDN is left with the state from "sending started".
Change
BaseMsgTrackingModulehands each event to a single-thread executor of its own. Writes stay off the processing thread and happen in the order the events were handled. The thread is a daemon, so it never keeps the JVM alive.ActiveModule, so the shutdown drain from Finish files being sent before shutting down, and report the ones cut off #625 already calls itsawaitIdlewith the shared deadline, after the pollers have finished their sends.awaitIdlenow waits until the events queued so far are written, so a message sent just before shutdown keeps its tracking record. (Previously those writes were on independent threads that nothing waited for.)DbTrackingModule.stopstops the writer, andstartgives a restarted module a new one. An event handled after the writer has stopped is written directly instead of being dropped.Your comment on #517 mentions moving tracking onto a queue in 5.0. This is a minimal version of that which fixes the ordering now, and it should fold into that work.
Tests
TrackingWriteOrderTestuses a recording tracker whosepersistis synchronized likeDbTrackingModule's:awaitIdle, so it checks only the order.awaitIdlewaits for the events already handled, and gives up at its timeout.session.drainWorkInProgresswaits for queued tracking events.Mutation checks: each of these changes fails at least one test:
awaitIdlenot waitingFull suite: 253 tests pass.