From ef7cb683a9b4172b6e89edd005771abeb96a8738 Mon Sep 17 00:00:00 2001 From: Asdine El Hrychy Date: Sat, 10 Oct 2020 12:45:49 +0400 Subject: [PATCH 1/5] Add rfc 001 --- rfcs/001_context.md | 147 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 147 insertions(+) create mode 100644 rfcs/001_context.md diff --git a/rfcs/001_context.md b/rfcs/001_context.md new file mode 100644 index 000000000..2a2aa9f84 --- /dev/null +++ b/rfcs/001_context.md @@ -0,0 +1,147 @@ +- Feature name: `Cancelable operations and use of Context` +- Date: 2020-10-10 +- Author: Asdine El Hrychy +- Related discussions: [Issue #224](https://github.com/genjidb/genji/issues/224), [PR #231](https://github.com/genjidb/genji/pull/231) + +## General approach + +I would like to avoid context pollution as much as possible, but without restricting the user flexibility. It is important to be able to control and cancel operations but I also believe that there are times where we don't really care about that. +I want Genji to be simple to use and intuitive, and for that I prefer to expand a bit more the surface API, rather than making the existing ones more complete and complex. +The purpose of this RFC is not to describe in details how each individual operation should be cancelled but rather which APIs should expect a context parameter. + +## Genji package and database lifecycle + +### Open and New + +Opening a database relies on IO and should be cancelable, but after that operation is completed the database handle itself is long lived and should not be cancelable, but closable with the `Close()` method. +However, most of the time, we don't care about how long it takes. + +We currently have two ways of opening a database: + +- `genji.Open` +- `genji.New` + +`Open` is just a specialized way of opening a database which calls `New`. It is by nature incomplete: + +- It only allows opening an in-memory database or a Bolt database +- It doesn't take any option + +```go +db, err := genji.Open("foo.db") +if err != nil { + return err +} +defer db.Close() +``` + +It is very simple to write and to use. I think it should remain as-is. + +If users want more power, there is the `genji.New` function. + +Opening a database can take time, even for Bolt or Badger, because accessing disks is unbounded (slow disks, NFS, etc.). It makes sense to have this a cancelable operation. + + + +```go +ctx, cancel := context.WithTimeout(context.Background(), 5 * time.Second) +... + +// ctx here is only used to control how long the engine creation can take +// as engines might wait for IO +ng := someengine.New(ctx, ...) +// but not all of them +ng := memoryengine.New() + +// at startup, Genji performs some operations on the engine, this should be cancelable +db, err := genji.New(ctx, ng) +``` + +Storing the context for further uses prevents doing that and is not really intuitive for a database handle that is supposed to be long lived. + +### genji.DB methods + +Each of these should be cancelable independently, at any given time while the database handle is opened. +Because cancelation is handled independently from the database handle, it is very intuitive and doesn't require preparing a different handle to be able to cancel a query. + +```go +db.Exec(ctx, "INSERT INTO foo(a) VALUES (1)") +db.Query(ctx, "SELECT 1") +db.QueryDocument(ctx, "SELECT 1") +``` + +Since a transaction is short-lived and atomic, contexts could be used to cancel the whole transaction by passing a context to `Begin`. +Any call to the transaction handle methods would return an error if the context is canceled. + +```go +tx, _ := db.Begin(ctx, true) +defer tx.Rollback() + +tx.Exec("INSERT INTO foo(a) VALUES (1)") +tx.Query("SELECT 1") +tx.QueryDocument("SELECT 1") + +tx.Commit() +``` + +This also applies to `db.View` and `db.Update`: + +```go +db.View(ctx, func(tx *Tx) error) error { + tx.Exec("INSERT INTO foo(a) VALUES (1)") + tx.Query("SELECT 1") + tx.QueryDocument("SELECT 1") +}) + +db.Update(ctx, func(tx *Tx) error) error { + tx.Exec("INSERT INTO foo(a) VALUES (1)") + tx.Query("SELECT 1") + tx.QueryDocument("SELECT 1") +}) +``` + +### The case of Close + +I don't think we should pass a context to `db.Close()`. Closing a database handle can take time, depending on the type of engine used and one might want to control that. +But most of the time this is not important, so I'd rather add a specialized method. + +```go +ctx, cancel := context.WithTimeout(context.Background(), 5 * time.Second) + +err = db.Shutdown(ctx) +``` + +## Engines + +Since almost every engine action is done from within a transaction, cancelation must be control by the context passed to the `Begin` method. Any action done on a canceled transaction must return an error. + +```go +ctx, cancel := context.WithTimeout(context.Background(), 5 * time.Second) + +// only the Begin method takes the context. +tx, err := ng.Begin(ctx, true) + +st, err := tx.GetStore([]byte("foo")) +err = st.Put([]byte("a"), []byte("b")) + +it := st.NewIterator(engine.IteratorConfig{}) +defer it.Close() +``` + +### Packages that use an engine + +Since the context is passed only to `engine.Engine#Begin`, only the functions that explicitely need to open a transaction must expect a context themselves. + +```go +package database + +func (db *Database) Begin(ctx context.Context, writable bool) (*Transaction, error) +func (db *Database) BeginTx(ctx context.Context, writable bool) (*Transaction, error) +``` + +Any function using the returned transaction must not expect a context in their signature. + +However, there might be some exceptions: If a function performs a long running operation or relies on IO itself, it must expect a context. + +### Other packages + +If a function doesn't rely on IO, it must not expect a context. From 087bbb43dc8b2488977e941ba4d1abb8c091ce78 Mon Sep 17 00:00:00 2001 From: Asdine El Hrychy Date: Sat, 10 Oct 2020 12:54:11 +0400 Subject: [PATCH 2/5] Fix typos --- rfcs/001_context.md | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/rfcs/001_context.md b/rfcs/001_context.md index 2a2aa9f84..045e86c82 100644 --- a/rfcs/001_context.md +++ b/rfcs/001_context.md @@ -5,15 +5,15 @@ ## General approach -I would like to avoid context pollution as much as possible, but without restricting the user flexibility. It is important to be able to control and cancel operations but I also believe that there are times where we don't really care about that. -I want Genji to be simple to use and intuitive, and for that I prefer to expand a bit more the surface API, rather than making the existing ones more complete and complex. -The purpose of this RFC is not to describe in details how each individual operation should be cancelled but rather which APIs should expect a context parameter. +I would like to avoid context pollution as much as possible, but without restricting user flexibility. It is important to be able to control and cancel operations but I also believe that there are times where we don't care about that. +I want Genji to be simple to use and intuitive, and for that, I prefer to expand a bit more the surface API, rather than making the existing ones more complete and complex. +The purpose of this RFC is not to describe in detail how each operation should be canceled but rather which APIs should expect a context parameter. ## Genji package and database lifecycle ### Open and New -Opening a database relies on IO and should be cancelable, but after that operation is completed the database handle itself is long lived and should not be cancelable, but closable with the `Close()` method. +Opening a database relies on IO and should be cancelable, but after that operation is completed the database handle itself is long-lived and should not be cancelable, but closable with the `Close()` method. However, most of the time, we don't care about how long it takes. We currently have two ways of opening a database: @@ -34,14 +34,12 @@ if err != nil { defer db.Close() ``` -It is very simple to write and to use. I think it should remain as-is. +It is very simple to write and to use, it must remain as-is. -If users want more power, there is the `genji.New` function. +If users want more power, they must use the `genji.New` function. Opening a database can take time, even for Bolt or Badger, because accessing disks is unbounded (slow disks, NFS, etc.). It makes sense to have this a cancelable operation. - - ```go ctx, cancel := context.WithTimeout(context.Background(), 5 * time.Second) ... @@ -56,11 +54,11 @@ ng := memoryengine.New() db, err := genji.New(ctx, ng) ``` -Storing the context for further uses prevents doing that and is not really intuitive for a database handle that is supposed to be long lived. +Storing the context for further uses prevents doing that and is not intuitive for a database handle that is supposed to be long-lived. ### genji.DB methods -Each of these should be cancelable independently, at any given time while the database handle is opened. +Each of these must be cancelable independently, at any given time while the database handle is opened. Because cancelation is handled independently from the database handle, it is very intuitive and doesn't require preparing a different handle to be able to cancel a query. ```go @@ -69,8 +67,8 @@ db.Query(ctx, "SELECT 1") db.QueryDocument(ctx, "SELECT 1") ``` -Since a transaction is short-lived and atomic, contexts could be used to cancel the whole transaction by passing a context to `Begin`. -Any call to the transaction handle methods would return an error if the context is canceled. +Since a transaction is short-lived and atomic, contexts must be used to cancel the whole transaction by passing a context to `Begin`. +Any call to the transaction handle methods must return an error and rollback the transaction if the context is canceled. ```go tx, _ := db.Begin(ctx, true) @@ -112,7 +110,7 @@ err = db.Shutdown(ctx) ## Engines -Since almost every engine action is done from within a transaction, cancelation must be control by the context passed to the `Begin` method. Any action done on a canceled transaction must return an error. +Since almost every engine action is done from within a transaction, cancelation must be controlled by the context passed to the `Begin` method. Any action done on a canceled transaction must return an error. ```go ctx, cancel := context.WithTimeout(context.Background(), 5 * time.Second) @@ -129,7 +127,7 @@ defer it.Close() ### Packages that use an engine -Since the context is passed only to `engine.Engine#Begin`, only the functions that explicitely need to open a transaction must expect a context themselves. +Since the context is passed only to `engine.Engine#Begin`, only the functions that explicitly need to open a transaction must expect a context themselves. ```go package database @@ -140,7 +138,7 @@ func (db *Database) BeginTx(ctx context.Context, writable bool) (*Transaction, e Any function using the returned transaction must not expect a context in their signature. -However, there might be some exceptions: If a function performs a long running operation or relies on IO itself, it must expect a context. +However, there might be some exceptions: If a function performs a long-running operation or relies on IO itself, it must expect a context. ### Other packages From bc1ca0981897b8a41d16fa3a381de60ce3258556 Mon Sep 17 00:00:00 2001 From: Asdine El Hrychy Date: Sat, 10 Oct 2020 13:08:03 +0400 Subject: [PATCH 3/5] Add small improvements --- rfcs/001_context.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/rfcs/001_context.md b/rfcs/001_context.md index 045e86c82..83180e016 100644 --- a/rfcs/001_context.md +++ b/rfcs/001_context.md @@ -14,7 +14,7 @@ The purpose of this RFC is not to describe in detail how each operation should b ### Open and New Opening a database relies on IO and should be cancelable, but after that operation is completed the database handle itself is long-lived and should not be cancelable, but closable with the `Close()` method. -However, most of the time, we don't care about how long it takes. +However, most of the time, we don't care about how long it takes to open a database. We currently have two ways of opening a database: @@ -54,8 +54,6 @@ ng := memoryengine.New() db, err := genji.New(ctx, ng) ``` -Storing the context for further uses prevents doing that and is not intuitive for a database handle that is supposed to be long-lived. - ### genji.DB methods Each of these must be cancelable independently, at any given time while the database handle is opened. @@ -118,6 +116,7 @@ ctx, cancel := context.WithTimeout(context.Background(), 5 * time.Second) // only the Begin method takes the context. tx, err := ng.Begin(ctx, true) +// Transaction methods don't need to take a context. st, err := tx.GetStore([]byte("foo")) err = st.Put([]byte("a"), []byte("b")) From faed8c6cafeb37b849ffb7e387e84e856c751a0c Mon Sep 17 00:00:00 2001 From: Asdine El Hrychy Date: Sat, 10 Oct 2020 14:46:39 +0400 Subject: [PATCH 4/5] Add WithContext --- rfcs/001_context.md | 55 +++++++++++---------------------------------- 1 file changed, 13 insertions(+), 42 deletions(-) diff --git a/rfcs/001_context.md b/rfcs/001_context.md index 83180e016..bff371afd 100644 --- a/rfcs/001_context.md +++ b/rfcs/001_context.md @@ -56,54 +56,25 @@ db, err := genji.New(ctx, ng) ### genji.DB methods -Each of these must be cancelable independently, at any given time while the database handle is opened. -Because cancelation is handled independently from the database handle, it is very intuitive and doesn't require preparing a different handle to be able to cancel a query. +By default, database methods must not expect a context. This avoids context pollution when cancelation is not necessary. +However, when it is necessary, a new handle can be created to use a specific context, using a new method named `WithContext`. ```go -db.Exec(ctx, "INSERT INTO foo(a) VALUES (1)") -db.Query(ctx, "SELECT 1") -db.QueryDocument(ctx, "SELECT 1") -``` - -Since a transaction is short-lived and atomic, contexts must be used to cancel the whole transaction by passing a context to `Begin`. -Any call to the transaction handle methods must return an error and rollback the transaction if the context is canceled. - -```go -tx, _ := db.Begin(ctx, true) -defer tx.Rollback() - -tx.Exec("INSERT INTO foo(a) VALUES (1)") -tx.Query("SELECT 1") -tx.QueryDocument("SELECT 1") - -tx.Commit() -``` +// non-cancelable queries +db.Exec("INSERT INTO foo(a) VALUES (1)") +db.Query("SELECT 1") +db.QueryDocument("SELECT 1") -This also applies to `db.View` and `db.Update`: - -```go -db.View(ctx, func(tx *Tx) error) error { - tx.Exec("INSERT INTO foo(a) VALUES (1)") - tx.Query("SELECT 1") - tx.QueryDocument("SELECT 1") -}) - -db.Update(ctx, func(tx *Tx) error) error { - tx.Exec("INSERT INTO foo(a) VALUES (1)") - tx.Query("SELECT 1") - tx.QueryDocument("SELECT 1") -}) -``` - -### The case of Close +ctx, cancel := context.WithTimeout(context.Background(), 5 * time.Second) -I don't think we should pass a context to `db.Close()`. Closing a database handle can take time, depending on the type of engine used and one might want to control that. -But most of the time this is not important, so I'd rather add a specialized method. +// WithContext returns a cancelable database handle +db.WithContext(ctx).Exec("INSERT INTO foo (a) VALUES (1)") -```go -ctx, cancel := context.WithTimeout(context.Background(), 5 * time.Second) +dbx := db.WithContext(ctx) -err = db.Shutdown(ctx) +// any called dbx method must return an error if the context is canceled. +dbx.Query("SELECT 1") +dbx.Begin(true) ``` ## Engines From afc2d12172ac1b59367f6edc0fab61db0f10ae95 Mon Sep 17 00:00:00 2001 From: Asdine El Hrychy Date: Sun, 25 Oct 2020 17:35:52 +0400 Subject: [PATCH 5/5] Add details about context cancellation in iterators Co-authored-by: Ivan Trubach --- rfcs/001_context.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/rfcs/001_context.md b/rfcs/001_context.md index bff371afd..2695ad91f 100644 --- a/rfcs/001_context.md +++ b/rfcs/001_context.md @@ -81,6 +81,8 @@ dbx.Begin(true) Since almost every engine action is done from within a transaction, cancelation must be controlled by the context passed to the `Begin` method. Any action done on a canceled transaction must return an error. +In addition, context cancellation or some unrecoverable I/O error may occur when iterating over store with `engine.Iterator`. In such case iterator is invalidated, `Valid()` method returns false and the new `Err()` will return non-nil error. + ```go ctx, cancel := context.WithTimeout(context.Background(), 5 * time.Second) @@ -93,6 +95,12 @@ err = st.Put([]byte("a"), []byte("b")) it := st.NewIterator(engine.IteratorConfig{}) defer it.Close() +for it.Seek(nil); it.Valid(); it.Next() { + // … +} +if err := it.Err(); err != nil { + return err +} ``` ### Packages that use an engine