Skip to content

[WIP] Proof of proof of concept for Tide-global state #100

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

Closed
wants to merge 2 commits into from

Conversation

bIgBV
Copy link
Contributor

@bIgBV bIgBV commented Nov 30, 2018

Description

Proof of concept for global state internal to Tide.

Motivation and Context

Currently we have the following form of state in Tide:

(Quoting @tirr-c ):

  • App-global across-middleware data at Data
  • App-global middleware-local data inside each Middleware
  • Request-local across-middleware data at extension (or wherever)
  • Request-local middleware-local data inside handle

But this does not provide a way to save state internal to Tide, but accessible globally within the framework itself, as an example design for #5.

This means that things which need to be accessible internally to Tide such as say a Config struct and a root logger are no longer accessible.

This PR provides a proof of concept by using a typemap wrapped in a reader-writer lock to provide synchronized access to this global state.

How Has This Been Tested?

I modified the logging middleware and added a dummy Config struct to demonstrate particular usecases for such a system. Right now if an example is run, we log a message mentioning what environment the server is running in and what address is it bound to.

The biggest issue I have with this design is:

Having to access the STATE variable directly.

I think a better design would be to wrap the STATE typemap with our own struct and provide a pub get method and a pub(crate insert method. This way we provide a way for the users of the framework to only read the application configuration, while internally within the framework we have read-write access.

This would mean that we can provide route level configuration as mentioned in #5 as well.

I tried to implement this in this design, but couldn't make it work with the current design.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

* Use Extesions from http crate to store tide-global(ish) state such as
config.
@tirr-c
Copy link
Collaborator

tirr-c commented Dec 2, 2018

If two Apps are created, they'll share the same state. It's not very likely to happen though... Is this intended?

@bIgBV
Copy link
Contributor Author

bIgBV commented Dec 2, 2018

@tirr-c no it isn't. I didn't realize that possibility. I think one way to handle this is to wrap it in a struct with a id of some sort and instantiate one for every app instance.

@bIgBV
Copy link
Contributor Author

bIgBV commented Dec 3, 2018

@aturon is the general idea behind an approach like this viable? I feel like this is something like this will be required for #60 as it ties directly into two major concerns, being logging and app configuration.

@bIgBV
Copy link
Contributor Author

bIgBV commented Dec 4, 2018

I've been trying to somehow add a Entry API to the underlying AnyMap which also includes handling locking, but it is a little beyond my rust skills 😅. I think it is possible, but the issue I'm running into right now is, with the AnyMap, you get Option<&T> or Option<T> values, which doesn't really work when trying to add an entry method to it.

On the other hand TypeMap exposes a Entry struct, but wrapping the map in a lock means that it you end up with something similar to the current API anyway.

Basically what I'm looking to return from functions such as get and insert is something along the lines of <RwLockReadGuard<Entry<K>> where K: Key and <RwLockWriteGuard<Entry<K>> where K: Key respectively.

The idea is to go from this in the insert case:

https://github.com/rust-net-web/tide/blob/082cbc02ce2d10c284d83c4e535756881cbff679/src/app.rs#L39-L42

to something along the lines of:

STATE.entry::<Config>().insert(config);

And for the read side to something along the lines of:

{
    let config = STATE.entry::<Config>().get();
    // Do something with `config` here
} // `config` gets dropped here and the lock is unlocked.

So the one way I can think of doing this is that instead of wrapping the AnyMap in a RwLock, we lock each bucket instead. That way, when calling get on an Entry, we can call read and return a RwLockReadGuard<Config>. At least I think that's a possibility, though I might be way off on that.

@bIgBV bIgBV mentioned this pull request Dec 4, 2018
@aturon
Copy link
Collaborator

aturon commented Dec 11, 2018

@bIgBV so I agree that the key missing thing right now is a way to communicate data with middleware and endpoints during the "static" phase (that is, for configuration). And I also agree that we'll ultimately want this configuration to be easily loadable from the external environment.

That said, there are a few reasons that I'd prefer to achieve these goals more along the lines of #5:

  • It allows for "static configuration" to be tweaked at any point in the router hierarchy, rather than just globally

  • It avoids the need to build in a global RwLock, because (in principle) you should have mutable access via the App during setup.

However, the key thing to making that work will be getting some variant of typemap working for configuration. I left a comment with some ideas here, lemme know what you think!

@bIgBV
Copy link
Contributor Author

bIgBV commented Dec 14, 2018

With the discussion in #5 and the new PR for configuration #109 , I feel this PR is no longer needed.

@bIgBV bIgBV closed this Dec 14, 2018
@bIgBV bIgBV deleted the app-global-state branch December 14, 2018 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants