search

real-logic / artio

Artio - Resilient High-Performance FIX and FIXP Gateway
Apache License 2.0
296 stars 121 forks source link

Customisable cancel on disconnect behaviour #482

Closed BrentDouglasB1 closed 1 year ago

BrentDouglasB1 commented 1 year ago

This changes providing a customizable cancel on disconnect option to an engine that will be applied in the case a login message does not provide one. The use case if for supporting an api that always uses cancel on disconnect and does not want to provide an option in the FIX api.

BrentDouglasB1 commented 1 year ago

For any future patches, is there a gradle task or something to apply the format from this repo automatically? I kept the format manually, but presumably you have some tooling for it that I couldn't see?

BrentDouglasB1 commented 1 year ago

@wojciech-adaptive Could you comment on what the chances are of something like this getting merged?

wojciech-adaptive commented 1 year ago

For any future patches, is there a gradle task or something to apply the format from this repo automatically? I kept the format manually, but presumably you have some tooling for it that I couldn't see?

I think the closest we have is this: https://github.com/real-logic/aeron/blob/master/config/ide/idea/aeron.xml

BrentDouglasB1 commented 1 year ago

@wojciech-adaptive I think have addressed all comments, though I am not super happy the javadoc change is particularly clear.

I'm not sure I understand your comment about initiator session though. In the earlier version of this PR I was erroneously using the configuration value for initiator sessions here: https://github.com/real-logic/artio/blob/master/artio-core/src/main/java/uk/co/real_logic/artio/engine/framer/Framer.java#L1561 and using the default rather than the session value in FixGatewaySessions#aquire, however I have fixed that now. Otherwise the only way I see it affecting initiator sessions is that if the acceptor has a FixDictionary that supports COD, they will get the acceptor configured defaults echoed back on the login, rather than the static defaults. Did you mean one of these things or is there another way that it affects initiator sessions that I am overlooking?

wojciech-adaptive commented 1 year ago

I'm not sure I understand your comment about initiator session though. In the earlier version of this PR I was erroneously using the configuration value for initiator sessions here: https://github.com/real-logic/artio/blob/master/artio-core/src/main/java/uk/co/real_logic/artio/engine/framer/Framer.java#L1561 and using the default rather than the session value in FixGatewaySessions#aquire, however I have fixed that now. Otherwise the only way I see it affecting initiator sessions is that if the acceptor has a FixDictionary that supports COD, they will get the acceptor configured defaults echoed back on the login, rather than the static defaults. Did you mean one of these things or is there another way that it affects initiator sessions that I am overlooking?

Yeah, COD callbacks aren't a thing for initiators, but they need to specify COD parameters in the Logon message, originally it was done using SessionCustomisationStrategy, with your previous change the engine defaults would also affect that. I think either is fine, but we should make sure to cover all cases in JavaDoc and tests. Sticking to SessionCustomisationStrategy for now seems good to me.

wojciech-adaptive commented 1 year ago

Merged, thanks. Can you please update https://github.com/real-logic/artio/wiki/Cancel-On-Disconnect-Notification accordingly?

BrentDouglasB1 commented 1 year ago

Thanks. I was looking at that before but I don't see how I would edit the wiki. Do I clone it and then somehow open a PR against the wikis repo?

wojciech-adaptive commented 1 year ago

My bad, thought anyone can edit the wiki. I'll update it.