algorand / algorand/go-algorand

tealdbg: Enforce critical regions for concurrent data access

Ouverte
#3,850 0 commentaires 0 réactions 0 personnes assignées Voir sur GitHub
new-feature-request Team Scytale
Langage dominant
Go
Étoiles
1.4k
Forks
537
Merge moyen
1 j 7 h
PR mergées (30 j)
18

Description

## Problem

As discussed in https://github.com/algorand/go-algorand/pull/3653#discussion_r834676699, `Debugger.Update` updates shared state _without_ acquiring a lock for the entire sequence of updates. Consequently, it's possible state will be partially updated and unexpectedly read/modified by another goroutine.

In practice, the chance for partial state update _seems_ low because tealdbg interaction happens slowly. A person stepping through a program is _probably_ moving slowly enough that the problem won't occur. However, future tealdbg changes are liable to make the problem more evident.

## Solution

* Lock reads/updates transactionally instead of per operation as is currently done. The entire sequence of shared state reads/writes must be guarded within a critical region.
* Consider if the locking mechanism provides re-entrant capabilities. Depending on the implementation, a reentrant lock is necessary.
* Remove atomics from `session.Line` in favor of critical regions like all other shared state.
* It's worth double checking _if_ a rationale exists for using atomics. From https://github.com/algorand/go-algorand/pull/3653#discussion_r834676699 and related verbals, no rationale was presented.

## Dependencies

N/A

## Urgency

TBD

Guide de contribution

Ouvrir le guide de contribution

Évaluation

Cette issue n'a pas encore été évaluée.

Recevez les nouvelles issues par e-mail

Un résumé court des issues GitHub adaptées aux débutants.