fix: stop accepting offers if zcf.shutdown is called #1772
Conversation
katelynsills
force-pushed
the
1756-stop-accepting-offers
branch
from
a70095f to
b3857cd
Compare
There was a problem hiding this comment.
LGTM
The one fix I suggest is for a behavior that is not new as of this PR, but which this PR caused me to notice. Therefore, it is ok if it is not fixed as of this PR. But I would like your thoughts on it. And please file as issue if you agree that this is a genuine problem.
| () => instanceAdmin.exitAllSeats(), | ||
| () => instanceAdmin.exitAllSeats(), | ||
| () => instanceAdmin.shutdown(), | ||
| () => instanceAdmin.shutdown(), |
There was a problem hiding this comment.
On seats, we have exit vs kickOut to distinguish between exiting for "normal" reasons vs exiting because of a problem. Such problems include circumstances in addition to the contract calling kickOut. For example, if an offerHandler's returned promise rejects, the seat is kicked out with the rejection reason, which is good.
IIUC, if the zcf vat crashes, the rejection handler above is called with an error representing the reason for crash. But the resulting shutdown ignores this reason and exits all the seats as if by exit rather than as if by kickOut. This is a false-successful-exit inconsistency.
Going the other way, on an explicit call to zcf.shutdown(), all the seats already created exit as if by exit rather than kickOut, which seems good. But any offer made after shutdown causes the offer to be rejected. We might view the later as a false-problematic-exit inconsistency. Or at least race condition.
I suggest that we fix the first inconsistency. But I suggest we document rather than "fix" the latter race condition, as I don't see a fix that's actually better than the current behavior.
I note that the one fix I suggest, to the rejection-handler behavior above, is not new as of this PR. Therefore, it is ok if it is not fixed as of this PR.
When we do address these, we may also want to introduce a shutdown variant that the contract can explicitly call to say "I'm shutting down abnormally because of the following problem", where that variant would also result in a kickOut of all seats.
There was a problem hiding this comment.
This sounds good to me. The shutdown variant you describe should be a good fix for the first issue, in that we can hook it up to the rejection handler.
There was a problem hiding this comment.
Issue created here: #1774
katelynsills
force-pushed
the
1756-stop-accepting-offers
branch
from
b3857cd to
6628d36
Compare
Closes #1756
This PR makes it such that Zoe no longer accepts offers for instances that have been shutdown