Skip to content

Commit

Permalink
oonitemplates: remove code causing crash on x86/arm64 (#360)
Browse files Browse the repository at this point in the history
The matter is better explained in the newly added code comment. I feel
okay removing the code, because that was a okay-lets-test-it piece of
code that was only useful for running such test.

A key takeaway from the investigation of this issue is that we really
want to simplify and streamline the way in which netx interacts with
the rest of the world, to avoid using channels where we can more easily
have (1) code that directly logs, if logging is the intent, and (2)
code that directly saves data, if that is the intent.

I will open an issue to document these changes more in depth. Here as
a general reflection, I'd just like to reiterate that if we were using
less complex/general channel based patterns, we would be reducing the
odds that we see some unexpected behaviour like in this case.

Another aspect is that we should have QA running everytime we do
generate a new Android build - see #358.

Closes #355
  • Loading branch information
bassosimone authored Feb 21, 2020
1 parent f4dcad9 commit 4e3bde3
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 22 deletions.
9 changes: 6 additions & 3 deletions internal/oonitemplates/oonitemplates.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import (
"net/http"
"net/url"
"sync"
"sync/atomic"
"time"

goptlib "git.torproject.org/pluggable-transports/goptlib.git"
Expand All @@ -32,7 +31,6 @@ import (

type channelHandler struct {
ch chan<- modelx.Measurement
lateWrites int64
}

func (h *channelHandler) OnMeasurement(m modelx.Measurement) {
Expand All @@ -44,7 +42,12 @@ func (h *channelHandler) OnMeasurement(m modelx.Measurement) {
select {
case h.ch <- m:
case <-time.After(100 * time.Millisecond):
atomic.AddInt64(&h.lateWrites, 1)
// We use to have code here but that caused crashes on
// the Android simulator under x86 and on devices. We
// don't have a solid theory of why it happens, but we
// know that _not_ putting code here prevents this from
// happening. The issue for tracking this bug was:
// https://github.com/ooni/probe-engine/issues/355.
}
}

Expand Down
19 changes: 0 additions & 19 deletions internal/oonitemplates/oonitemplates_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,33 +5,14 @@ import (
"errors"
"net"
"strings"
"sync"
"testing"
"time"

goptlib "git.torproject.org/pluggable-transports/goptlib.git"
"github.com/ooni/netx/modelx"
"gitlab.com/yawning/obfs4.git/transports"
obfs4base "gitlab.com/yawning/obfs4.git/transports/base"
)

func TestUnitChannelHandlerWriteLateOnChannel(t *testing.T) {
handler := &channelHandler{
ch: make(chan modelx.Measurement),
}
var waitgroup sync.WaitGroup
waitgroup.Add(1)
go func() {
time.Sleep(1 * time.Second)
handler.OnMeasurement(modelx.Measurement{})
waitgroup.Done()
}()
waitgroup.Wait()
if handler.lateWrites != 1 {
t.Fatal("unexpected lateWrites value")
}
}

func TestIntegrationDNSLookupGood(t *testing.T) {
ctx := context.Background()
results := DNSLookup(ctx, DNSLookupConfig{
Expand Down

0 comments on commit 4e3bde3

Please sign in to comment.