16
20
Derivar 16

Protocol Engine Refactor #181

Integrado
dan integrou 1 cometimento(s) do ramo protocolengine no ramo master 2019-01-09 20:24:57 +00:00
Proprietário(a)

Very large refactor:

  • Broken out Connections and other classes into Protocol
  • Integrated Peer with EventBus

A few things that are still weird:

  • Status Updates - Currently Peer still has direct function call dependence on Protocol Engine which is how it gets status updates (peer/server connection status) - eventually these should be event bus messages
  • Alice/Bob Peer Examples don't work, these need to be updated to use EventBus
  • Peer unit tests are non existent, one nice thing is we can now test a lot more because of the event bus.

Nevertheless, take a look as it likely impacts your integrations. Everything works as is, and the old CwtchPeer interface is mostly respected (I didn't have to change much of the cli at all - however, other integrations that rely on AIF behavior will be broken, as that is one more area that needs tidying)

Very large refactor: * Broken out Connections and other classes into Protocol * Integrated Peer with EventBus A few things that are still weird: * Status Updates - Currently Peer still has direct function call dependence on Protocol Engine which is how it gets status updates (peer/server connection status) - eventually these should be event bus messages * Alice/Bob Peer Examples don't work, these need to be updated to use EventBus * Peer unit tests are non existent, one nice thing is we can now test a lot more because of the event bus. Nevertheless, take a look as it likely impacts your integrations. Everything works as is, and the old CwtchPeer interface is mostly respected (I didn't have to change much of the cli at all - however, other integrations that rely on AIF behavior will be broken, as that is one more area that needs tidying)
Membro
Drone Build Status: success https://build.openprivacy.ca/cwtch.im/cwtch/332
Proprietário(a)

just taking an early look
any reason peer.Init doesnt take a manager and create an eventBut internally? just looking at the integ tests and you had to create a whole bunch of busses manually and pass them in then.

then you could also wire eventBus.Shutdown into peer.Shutdown
seems like a cleaner API

aaaah. so in the app code i see app just has one eventBus and shares it with all the peers. Do events properly indicate what peer they are about? because if so the integ tests only need 1 event bus to share with all 3 peers, and if not then I think multi user is broken in the app

just taking an early look any reason peer.Init doesnt take a manager and create an eventBut internally? just looking at the integ tests and you had to create a whole bunch of busses manually and pass them in then. then you could also wire eventBus.Shutdown into peer.Shutdown seems like a cleaner API aaaah. so in the app code i see app just has one eventBus and shares it with all the peers. Do events properly indicate what peer they are about? because if so the integ tests only need 1 event bus to share with all 3 peers, and if not then I think multi user is broken in the app
Autor(a)
Proprietário(a)

just taking an early look any reason peer.Init doesnt take a manager and create an eventBus internally?

Because the eventBus is/(will be eventually) shared between all components in the system and so it's lifecycle should not be managed by any particular subsystem.

just looking at the integ tests and you had to create a whole bunch of busses manually and pass them in then.

because each peer in the integ case is representative of a seperate peer and so has a different bus. It's more accurate that these are tested with seperate event buses.

Do events properly indicate what peer they are about?......and if not then I think multi user is broken in the app

Yeah this is a good point, though multi-user isn't broken...it will still work, but there will be a lot of redundant work & errors that are not really errors (e.g. trying to listen to the same onion twice). Events could indicate intended subsystem/peer (or we could have different event registrations per peer)

I don't have a strong opinion either way, something we can dive into on Monday.

> just taking an early look any reason peer.Init doesnt take a manager and create an eventBus internally? Because the eventBus is/(will be eventually) shared between all components in the system and so it's lifecycle should not be managed by any particular subsystem. > just looking at the integ tests and you had to create a whole bunch of busses manually and pass them in then. because each peer in the integ case is representative of a seperate peer and so has a different bus. It's more accurate that these are tested with seperate event buses. > Do events properly indicate what peer they are about?......and if not then I think multi user is broken in the app Yeah this is a good point, though multi-user isn't broken...it will still work, but there will be a lot of redundant work & errors that are not really errors (e.g. trying to listen to the same onion twice). Events could indicate intended subsystem/peer (or we could have different event registrations per peer) I don't have a strong opinion either way, something we can dive into on Monday.
Proprietário(a)
  • event types should be enum
  • the discuessed new eventbus chaining thing for app

otherwise lgtm

- event types should be enum - the discuessed new eventbus chaining thing for app otherwise lgtm
Proprietário(a)

lgtm!

lgtm!
Autor(a)
Proprietário(a)

Most recent change:

  • Adds Event Type Enum
  • Kills AIF
  • Upgraded Examples for Alice Bob p2p Messaging
Most recent change: - Adds Event Type Enum - Kills AIF - Upgraded Examples for Alice Bob p2p Messaging
Membro
Drone Build Status: success https://build.openprivacy.ca/cwtch.im/cwtch/334
Membro
Drone Build Status: success https://build.openprivacy.ca/cwtch.im/cwtch/336
dan fechou este pedido de integração 2019-01-09 20:24:57 +00:00
Inicie a sessão para participar neste diálogo.
Sem revisores
Sem etapa
Sem encarregados
4 Participantes
Notificações
Data de vencimento
A data de vencimento é inválida ou está fora do intervalo permitido. Por favor, use o formato 'aaaa-mm-dd'.

Sem data de vencimento definida.

Dependências

Não estão definidas dependências.

Referência: cwtch.im/cwtch#181
Nenhuma descrição fornecida.