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

Improve autoclosable doc #42

Open
Emily-Jiang opened this issue Jan 11, 2018 · 6 comments
Open

Improve autoclosable doc #42

Emily-Jiang opened this issue Jan 11, 2018 · 6 comments
Milestone

Comments

@Emily-Jiang
Copy link
Member

Emily-Jiang commented Jan 11, 2018

add an example
add more explanation

@jmesnil
Copy link
Contributor

jmesnil commented Jan 11, 2018

A typical ConfigSource example would be a ConfigSource that uses a DB to store its props.
For performance reason, we want to reuse a JDBC connection and closes it when the ConfigSource is not longer used.

However we have edge cases when using the programmatic API as such a ConfigSource can belong to multiple Configs.
If that's the case, from the ConfigSource implementation POV, I have no idea when my close method is called if there are other Configs that still uses it.
And I can not use reference counter since I can't know how many Configs uses it.

I wonder if we shouldn't have a symmetrical lifecycle init methods.
So that when a Config includes me, it calls my init() and when it releases me, it calls my close()
This way, the ConfigSource could know when it is no longer used at all and be able to free its resources when the last Config has released it.

The same argument can also be made for Converter.
For example, I have a GPSConverter that takes a GPS coordinates String and calls a DB to get the data and creates an Address object.
This converter needs to hold stateful resources (the DB connections). It needs also to know when it can release them.
As for ConfigSource, a Converter can be hold by many Configs (using the programmatic API).

I propose we add an empty default init() method to ConfigSource and Converter API.
These methods will always be called when a ConfigSource or a Converter are added to a Config.

@kazumura
Copy link

I wonder it may be implementation issue.
If an implementation allow ConfigSource to be shared by multiple Config,
can this implementation handle the edge case by their own way ?

@jmesnil
Copy link
Contributor

jmesnil commented Jan 18, 2018

The spec allows (or at least does not forbid) to share ConfigSources between Config using the programmatic ConfigBuilder.

The actual sentence in the spec is not correct:

If a ConfigSource implements the java.lang.AutoCloseable interface then the close() method will be called when the underlying Config is being released.

There is no Config under an ConfigSource (that's the other way around).
The relation are:

  • 1 Config has many ConfigSource
  • 1 ConfigSource belongs to many Config

I'd be fine with requiring Config implementation to manage this case but it should be explicitly stated in the spec something like:

If a ConfigSource implements the java.lang.AutoCloseable interface then its close() method will be called when the last Config that provides it is released.

@Emily-Jiang
Copy link
Member Author

The more we talk about this, the more I think the close is implementation detail. Maybe we just mention in the spec that the implementors need to ensure when a config object is released, the underline config source or converters should be cleared up.

@struberg
Copy link
Contributor

@Emily-Jiang Do you think we should still add any additional clarification or hint to the spec?
Or do you (also) think we are good now and whoever implements such edge cases has to deal with them himself?

@Emily-Jiang
Copy link
Member Author

I think it will be good to add additional clarification.

@struberg struberg added this to the EDR-1 milestone May 24, 2018
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

No branches or pull requests

4 participants