Skip to content

Monitor usage in QUICConnection #48

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
CMCDragonkai opened this issue Aug 21, 2023 · 3 comments · Fixed by #49
Closed

Monitor usage in QUICConnection #48

CMCDragonkai opened this issue Aug 21, 2023 · 3 comments · Fixed by #49
Assignees
Labels
development Standard development r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices

Comments

@CMCDragonkai
Copy link
Member

CMCDragonkai commented Aug 21, 2023

Specification

The monitor is necessary to enable re-entrancy within a call graph of certain operations like send and recv in QUICConnection while also ensuring mutual exclusion between different call graphs.

However, the monitor currently as used isn't using the monitor resource acquisition function that I had earlier created in MatrixAI/js-contexts#2.

I just pushed out 1.2.0 of js-contexts which exposes monitor as a ResourceAcquire.

You can remove the utils.withMonitor in favour of doing something like this:

await withF(monitor(this.lockBox, RWLockWriter), async ([mon]) => {
  await x.f({ monitor: mon });
});

Additionally in the beginning during QUICConnection.createQUICConnection, you don't actually need to setup so much just to do the initial recv and send under after start.

Also you don't need to lock anything in that scenario cause nothing is actually going to be able to intercept those calls during createQUICConnection because nothing else has reference to the QUICConnection instance.

I think it would far more clearer to write it in a way where your initial recv and send is just part of the async start method. Just put your recv and send calls within that. It does not appear that start is called anywhere else, so it can just be part of QUICConnection.start.

Finally both send and recv should have mon?: Monitor. Both should then use the withF pattern above in case the monitor doesn't yet exist.

Additional context

Tasks

  1. Use 1.2.0 of js-contexts to get access to the monitor function.
  2. Replace utils.withMonitor with monitor
  3. Use withF pattern to create monitor for both send and recv
  4. Incorporate initial recv and send within QUICConnection.start, since that would be much clearer. You can still setup a monitor, but no locking is really required at that point.
  5. In the QUICServer, why is there an additional separation of connectionProm? This is unnecessary.
@CMCDragonkai CMCDragonkai added the development Standard development label Aug 21, 2023
@CMCDragonkai
Copy link
Member Author

Regarding 5. it can look like this:

    let connection: QUICConnection;
    try {
      connection = await QUICConnection.createQUICConnection({

@CMCDragonkai
Copy link
Member Author

@tegefaulkes do you have a PR that tracks this?

@tegefaulkes
Copy link
Contributor

tegefaulkes commented Aug 22, 2023

No PR, It was small enough that I was going to push directly to staging.

Now that I think about it, It could use a PR since we're addressing multiple small changes.

  1. Fixing up monitor usage
  2. Applying startup timeout to server connections.
  3. Removing constraint between startup timeout and idle timeout

Am I missing anything?

tegefaulkes added a commit that referenced this issue Aug 22, 2023
@tegefaulkes tegefaulkes mentioned this issue Aug 22, 2023
9 tasks
@CMCDragonkai CMCDragonkai added the r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices label Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development Standard development r&d:polykey:core activity 4 End to End Networking behind Consumer NAT Devices
Development

Successfully merging a pull request may close this issue.

2 participants