Repository navigation
Write message tracking events in order on one thread - #638
Open
JakeHuneau wants to merge 1 commit into
Open
JakeHuneau wants to merge 1 commit into
JakeHuneau wants to merge 1 commit into
Conversation
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.
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.