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

Deterministic behavior for transactionId validation #1488

Closed
soareschen opened this issue Jul 17, 2017 · 4 comments · Fixed by #1521
Closed

Deterministic behavior for transactionId validation #1488

soareschen opened this issue Jul 17, 2017 · 4 comments · Fixed by #1521
Assignees

Comments

@soareschen
Copy link
Contributor

soareschen commented Jul 17, 2017

Currently the steps are as follows:

getParameters

  • transactionId is set to a new unique identifier, used to match this getParameters call to a setParameters call that may occur later.

setParameters
7. If parameters.encodings.length is different from N, or if any parameter in the parameters argument, marked as a Read-only parameter, has a value that is different from the corresponding parameter value returned from sender.getParameters(), abort these steps and return a promise rejected with a newly created InvalidModificationError. Note that this also applies to transactionId.
10. Set sender's internal transactionId slot to a previously unused value.

The specified rules have not been very consistent:

  • getParameters() does not store transactionId to any internal slot.
  • setParameters() says transactionId validation is applied the way same as parameters.encodings, but the field is outside of parameters.encodings. (Update: Misread the sentence, it applies to all fields in parameters)
  • setParameters() set transactionId to a new value, but that is overridden again when calling getParameters().

To fix this, a proper internal slot should be defined and transactionId validation in setParameters() should be done in separate steps.

@soareschen
Copy link
Contributor Author

soareschen commented Jul 17, 2017

The transactionId validation should also take into account of async race conditions and make sure that sequence of synchronous calls to get/set parameters have well defined behavior. e.g.:

const param = sender.getParameters();
const promise = sender.setParameters(param);
sender.getParameters();

promise.then(...); // resolve or reject?

Also note that the validation of parameters.encodings and any other fields should not have to rely on sender.getParameters(), as that would result in new transaction ID being generated and make the following transactionId validation behavior undefined.

@aboba aboba changed the title transactionId validation for get/set parameters should have more consistent definition Deterministic behavior for transactionId validation Jul 27, 2017
@aboba aboba assigned aboba and taylor-b and unassigned aboba Jul 27, 2017
@taylor-b
Copy link
Contributor

Also note that the validation of parameters.encodings and any other fields should not have to rely on sender.getParameters(), as that would result in new transaction ID being generated and make the following transactionId validation behavior undefined.

Do you mean that step 7 in the setParameters() algorithm shouldn't actually invoke the getParameters() algorithm? I don't think that was ever the intention; it was just written that way for simplicity.

@taylor-b
Copy link
Contributor

taylor-b commented Aug 9, 2017

Made a new issue for the race condition, since it's a bit more general than just transactionId: #1520

@taylor-b
Copy link
Contributor

Not completely closed by that PR, see issue #1520.

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

Successfully merging a pull request may close this issue.

3 participants