Skip to content

Context package specification#30

Merged
jcp19 merged 4 commits intomasterfrom
context-spec
Jun 20, 2022
Merged

Context package specification#30
jcp19 merged 4 commits intomasterfrom
context-spec

Conversation

@gavinleroy
Copy link
Contributor

Here is the initial outline of the specification of the context package. There are still things to be changed and some main questions lie in the following areas:

  • Should the Done method be labeled as pure? The downside is we cannot introduce the ghost return value indicating whether or not the channel was previously closed. The specification for the returned channel also needs to be refined but I am ignoring that for now.

  • The Err method internally obtains and releases a mutex. A Context is safe to pass to multiple goroutines and from the view of the caller, could in theory only require read access to the Mem predicate. The question here is if that is actually sound.

  • The Value method (and consequently WithValue function) currently requires that the key be a primitive type. However, according to the Go docs using a primitive type should be avoided and in the Scion project, the keys are only used with a new type declaration similar to the below snippet. Thus, enforcing a primitive type will not work.

type loggerContextKey string
  • The last three functions: WithDeadline, WithTimeout, and WithCancel all return a cancel function. We would need support for closures in order to specify this (I think). What I am most unsure about is, that calling this function should release all resources held by the context but this effect would also change the return values of the methods Err, and Done.

@jcp19 jcp19 requested review from Dspil and jcp19 June 17, 2022 14:01
ensures acc(parent.Mem(), definitions.ReadL20)
ensures acc(child.Mem(), definitions.ReadL20)

WithCancel(parent Context) (child Context, cancel CancelFunc)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment/question in the function WithValue about child and parent possibly being aliased also applies here. Here however, I would fix the spec but comment the function until we have a closure implementation, otherwise Gobra will complain about a missing definition for CancelFunc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As stated, the function WithCancel, WithDeadline, and WithTimeout are currently commented out due to unsupported closures. But the conversation at #30 (comment) still applies to the Parent-Child context relationships.

Comment on lines +129 to +136
// ---

requires acc(parent.Mem(), definitions.ReadL20)

ensures acc(parent.Mem(), definitions.ReadL20)
ensures acc(child.Mem(), definitions.ReadL20)

WithDeadline(parent Context, d time.Time) (child Context, cancel CancelFunc)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comments as for WithCancel and WithValue

ensures acc(parent.Mem(), definitions.ReadL20)
ensures acc(child.Mem(), definitions.ReadL20)

WithTimeout(parent Context, timeout time.Duration) (child Context, cancel CancelFunc)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as for WithValue and the other similar functions


// ---

// XXX! used frequently but not in the router
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it used in some dependency of the router?

…l permission due to potential aliasing. Giving up child permissions should grant you parent permission again with a magic wand.
Copy link
Collaborator

@jcp19 jcp19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I have some minor nitpicks that should be addressed before merging. Otherwise, we can merge if @Dspil agrees

preserves acc(Mem(), definitions.ReadL20)
requires acc(key.Mem(), _)
requires isComparable(key)
ensures acc(key.Mem(), _)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary: the precondition requires acc(key.Mem(), _) causes a positive amount of permissions (less than the caller holds) to be transferred to the callee, which implies that the caller always retains acc(key.Mem(), _) - as such, no need to transfer them back

requires acc(val.Mem(), _)
ensures child.Mem()
ensures child.Mem() --* parent.Mem()
ensures acc(val.Mem(), _)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also not needed

or interface.
*/

preserves acc(parent.Mem(), definitions.ReadL20)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm maybe we do not need to provide full permission to both parent and child - maybe, any positive permission amount p is good enough, as long as it is the same for parent and child and as long as the magic wand is changed to use that fraction on both side. Nonetheless, my intuition for requiring full permission is that you shouldn't access directly a Context that is an internal Context of another - you should only do this after knowing that the outermost Context is no longer used. I would keep it as it is and change in the future if we require a more expressive contract

preserves acc(Mem(), definitions.ReadL20)
ensures isClosed ==> c.Closed()
ensures (!isClosed && c != nil) ==>
acc(c.RecvChannel(), definitions.ReadL20)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a very subtle point that I just noticed is that c.RecvChannel() is not duplicable. More than that, c1.RecvChannel() together with c2.RecvChannel() implies that c1 and c2 must be different

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would simply using a wildcard permission satisfy this unsoundness on line 38?

Copy link
Collaborator

@jcp19 jcp19 Jun 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would prevent it, yes

@gavinleroy gavinleroy changed the title WIP Context package specification Context package specification Jun 20, 2022
@jcp19 jcp19 merged commit 17bdd32 into master Jun 20, 2022
@jcp19 jcp19 deleted the context-spec branch June 20, 2022 12:20
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