Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add two failing test cases, fix one. #38

Merged
merged 1 commit into from
Jan 28, 2015
Merged

Add two failing test cases, fix one. #38

merged 1 commit into from
Jan 28, 2015

Conversation

edsko
Copy link
Contributor

@edsko edsko commented Jun 10, 2014

The test suite examples/errors/Exceptions contains two different kinds of exceptions:

  1. The transaction contains an explicit fail (or error) but can still be serialized (failEvent, errorEvent)
  2. The transaction returns an undefined state (stateError).

This pull request introduced two further classes of errors, and fixes one of them:

  1. The transaction returns a state which is partially defined: defined in the spine but containing nested errors (stateNestedError1). This is the class of error which this pull request does not fix (see below).
  2. The transaction itself cannot be serialized (stateNestedError2 with an error value as argument).

For the latter error, scheduleLocalUpdate calls pushEntry on the encoding of the transaction to write the transaction to the log, but this encoding happens lazily. This means that the exception will happen in the file writer thread spawned by openFileLog; in particular, it will happen in fileWriter on the call to writeToDisk, which means the subsequent call to sequence_ action never happens. This means that the call to putMVar in scheduleLocalUpdate never happens and hence the call to takeMVar in update will be forever blocked.

We can fix this by reenabling the call to evaluate that was commented out in scheduleLocalUpdate, which is what this pull request does. This will ensure that the exception happen in this thread, and the problem is avoided.

The third problem mentioned above (exemplified by test stateNestedError1), however, is trickier to avoid. In this case any update will go through fine, but we will have a similar problem when we try to write a checkpoint (createLocalCheckpoint as well as createCheckpointAndClose). In this case, the state contains a nested error, so when the file writer thread attempts to write out the entire state as part of the Checkpoint it will again throw an exception, and again the putMVar never happens. A similar fix to the one for scheduleLocalUpdate would be possible, of course, simply by calling evaluate on the encoded state. However, unlike individual transactions, insisting that the full bytestring for the entire database state is loaded into memory before it is written to disk might be a bad idea.

Instead, it is probably necessary to adapt fileWriter to catch exceptions. This probably means it needs to get two lists of actions to execute: one on successful writing, one for exception cases. However, this is a bigger change so I didn't want to make it without discussion. (Once such a change is made, the call to evaluate in scheduleLocalUpdate will be obsolete).

(As an aside, the call to pushEntry in scheduleLocalColdUpdate may be suspect, too.)

@dmjio
Copy link
Member

dmjio commented Jun 25, 2014

ping @lemmih

@lemmih
Copy link
Member

lemmih commented Jun 25, 2014

Still on vacation. I defer all judgement to you, David.

@edsko
Copy link
Contributor Author

edsko commented Jan 28, 2015

Ping

lemmih added a commit that referenced this pull request Jan 28, 2015
Add two failing test cases, fix one.
@lemmih lemmih merged commit b5f4abf into acid-state:master Jan 28, 2015
@lemmih
Copy link
Member

lemmih commented Jan 28, 2015

I'm sad about the performance hit but I guess correctness is more important.

@edsko
Copy link
Contributor Author

edsko commented Jan 28, 2015

Thanks, and yes, agreed. My suggestion for fileWriter might help with performance though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants