search

ssbc / ssb-ebt

secure scuttlebutt replication with epidemic-broadcast-trees
MIT License
18 stars 10 forks source link

Avoid fallback replication if stream is closed #2

Closed clehner closed 5 years ago

clehner commented 7 years ago

Triggering fallback replication when the stream is closing can trigger deadlock where the peer stream & rpc object don't fully close. This should be fixed by https://github.com/ssbc/packet-stream/pull/9 also, which will trigger an error to get passed immediately to the legacy repliation handler in this case, but fixing this here will prevent that error from getting thrown in the first place. %gJ9LjBEIPdEBx/CDimsi3rtlJFOy7og/wqWQahzkpls=.sha256 has more info.

There are different types of errors being handled here:

  1. EBT is not supported. Error messages that i have seen for this are 'no duplex:ebt,replicate' and 'method:ebt,replicate is not on whitelist'. We should trigger legacy replication in this case.

  2. We closed the stream. Error message "unexpected end of parent stream". This error message also be synthesized locally in response to the remote stream closing, but it has basically the same effect. We do not need to trigger legacy replication in this case.

  3. The steam closed, because of a connection failure, or the remote broke the stream. Error message may be 'read ECONNRESET' or 'write EPIPE' or something else. I am assuming that any error with property syscall indicates a stream broken at a low level. We do not need to trigger legacy replication in this case.

  4. EBT failed but I don't know why. I'm assuming this is an internal EBT error which means we should trigger legacy replication.

I also made the "replication ended" message log just the error message instead of the whole stack, since this error seems to always be the same error handle in the other callback.

dominictarr commented 7 years ago

I feel uneasy about checking the error messages, this means that the error message is now part of the api, and that we can't change the error message (to something more informative) without breaking this. If we want to check error messages, we should have fixed error codes for that, not human readable strings.

What about having the legacy fallback just check if the rpc connection is still open, and bail out if not. I guess packet-stream@2.0.2 already achives that.

ahdinosaur commented 7 years ago

If we want to check error messages, we should have fixed error codes for that, not human readable strings.

:+1:

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.