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

Expose run_result_t publicly #106

Closed
Tectu opened this issue Jan 15, 2022 · 4 comments
Closed

Expose run_result_t publicly #106

Tectu opened this issue Jan 15, 2022 · 4 comments
Labels
discussion General discussions

Comments

@Tectu
Copy link
Owner

Tectu commented Jan 15, 2022

@0x00002a A typical consuming application of malloy::server would use start() to create a "session" and then destroys it when the server is supposed to be shutdown. In our examples we're currently just relying on the returned run_result_t object to go out of scope which will automatically shut down the malloy::server infrastructure.

Currently, unless I am missing something, there is no way for a consuming application to properly store the run_result_t object as a class member as we're not exposing the run_result_t type publicly.
Is there some caveat I'm currently not aware of or can we expose that type publicly so a consuming application could have a class member like std::optional<run_result_t> to control the state of the malloy server?

@Tectu Tectu added the discussion General discussions label Jan 15, 2022
@0x00002a
Copy link
Contributor

We could expose the alias certainly. The actual controller_run_result should stay private tho imo. Also we need to make it movable (as it currently isn't implicitly, since it has a custom dtor), it was an oversight on my part since if we're going to return a resource handle by value we at least need to allow some form of preserving its lifetime for the user.

Making it movable should be a simple case of adding = default to the move ctor and assignment and adding a check in the dtor for if m_io_ctx is nullptr (i.e. its been moved from). Also should add std::movableto the constraint on T in controller_run_result as its stored by value

@Tectu Tectu closed this as completed in 5874a5a Jan 16, 2022
@0x00002a
Copy link
Contributor

0x00002a commented Jan 16, 2022

I've just finished implementing this so I'll open a pr and we can discuss it there? I like the idea of some kind of session naming, but for that I suggest we move more of client::controller into it, since currently it the controller is still used on the client side to make new connections even after its started running (unlike the server which cannot dynamically add routes after its started, at least currently - not sure if we want this?). We could do this in a similar manner to the websocket connection, where certain functions are disabled depending on if its server or client session.

Edit:
@Tectu 5874a5a has a subtle bug, if you move the object before the threads have properly started it will cause a null pointer deref. Shall I still open that pr?

@Tectu
Copy link
Owner Author

Tectu commented Jan 16, 2022

Ah, unfortunate timing. I just closed this and opened #107 :p

Shall we discuss over there?

@Tectu
Copy link
Owner Author

Tectu commented Jan 16, 2022

@Tectu 5874a5a has a subtle bug

Uh-oh - what did I screw up?

We could do this in a similar manner to the websocket connection, where certain functions are disabled depending on if its server or client session.

I think that is a good approach.

Tectu added a commit that referenced this issue Jan 30, 2022
Related to #106 and #107.

* feat: make controller_run_result movable

* chore: improve tests slightly

* fix: accidental removal of m_workguard from controller_run_result

This actually was informative because it showed how m_workguard actually
does matter. See if there is no work guard, and the user calls
run() on a _client_ controller, then enough time elapses so that
all work is complete and there is a pause before new work is added,
the io_context::run() will return for each of the threads, meaning that
in actual fact queuing more actions via the controller (which is valid
after calling start) would not actually do anything.

tl;dr nasty obscure bug, keep the workguard its needed for clients

* feat: add start_result alias to controllers

* coding style

* chore: start_result -> session

* chore: server::controller -> server::routing_context

* chore: server/controller -> server/routing_context

* fix: tests include

* fix: more rename fixes

* fix: session test

* fix example server/applications/demo01

Co-authored-by: Joel Bodenmann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion General discussions
Projects
None yet
Development

No branches or pull requests

2 participants